- 
                Notifications
    You must be signed in to change notification settings 
- Fork 10
[APPS-7700] Add validation for scopes attribute for secure param #404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds validation for the scopes attribute on secure parameters in app manifests. The validation ensures scopes are only used with secure parameters, cannot be empty arrays, and contain only valid scope values from a predefined list.
Key Changes:
- Added validate_scopes_for_secure_parameterflag to control scope validation
- Implemented three validation rules: scopes require secure=true, scopes cannot be empty, and scopes must be from allowed values
- Added comprehensive test coverage for all validation scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description | 
|---|---|
| lib/zendesk_apps_support/validations/manifest.rb | Implements scope validation logic with three new error checks and adds SECURE_PARAM_SCOPES constant | 
| lib/zendesk_apps_support/manifest/parameter.rb | Adds scopesto parameter attributes to support reading scope configuration | 
| spec/validations/manifest_spec.rb | Adds comprehensive test coverage for scope validation scenarios including edge cases | 
| config/locales/translations/zendesk_apps_support.yml | Adds translation keys for three new validation error messages | 
| config/locales/en.yml | Adds English text for the three new validation error messages | 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
1fc8a24    to
    c5c1530      
    Compare
  
    c5c1530    to
    eaa1381      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
eaa1381    to
    1a093cb      
    Compare
  
    1a093cb    to
    7f2d771      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7f2d771    to
    8adf80d      
    Compare
  
    8adf80d    to
    d50c805      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d50c805    to
    866fd65      
    Compare
  
    | next if parameter.scopes.nil? | ||
|  | ||
| errors << ValidationError.new(:field_contains_invalid_attributes, | ||
| field: "parameters.[name=#{parameter.name}]", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the notation without . is closer to the array notation for JSON objects. Adding " as well to indicate it's a value might help.
This is kind of a hybrid between a JSON path and XPATH notation 😄. What do you reckon?
| field: "parameters.[name=#{parameter.name}]", | |
| field: "parameters[name=\"#{parameter.name}\"]", | 
866fd65    to
    a40095b      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a40095b    to
    9835af9      
    Compare
  
    9835af9    to
    acdef7c      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
acdef7c    to
    99670c9      
    Compare
  
    99670c9    to
    07d926b      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
16d174a    to
    3878082      
    Compare
  
    3878082    to
    04bdf10      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
spec/validations/manifest_spec.rb:10
- Corrected spelling of 'default ocale' to 'default locale'.
      'defaultLocale' => 'default ocale',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
04bdf10    to
    bf1777a      
    Compare
  
    
💐
/cc @zendesk/wattle
Description
Add validation for scopes attribute for secure param.
Validations added are as follows
nilvalidate_scopes_for_secure_parameterpassed in paramReferences
https://zendesk.atlassian.net/browse/APPS-7700
Risks