Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix incomplete-sanitization findings with regex #739

Closed
dlpzx opened this issue Sep 7, 2023 · 0 comments · Fixed by #762
Closed

Fix incomplete-sanitization findings with regex #739

dlpzx opened this issue Sep 7, 2023 · 0 comments · Fixed by #762
Assignees
@dlpzx
Labels
effort: low priority: high type: enhancement Feature enhacement
Milestone
v2.1.0

Comments

@dlpzx
Copy link
Contributor

dlpzx commented Sep 7, 2023

Is your idea related to a problem? Please describe.
From several scanning tools we are receiving the following error. Description: session.getIdToken().payload['custom:saml.groups'].replacemethod will only replace the first occurrence when used with a string argument ('['). If this method is used for escaping of dangerous data then there is a possibility for a
bypass.Try to use sanitization library instead or use a Regex with a global flag. Details: https://sg.run/1GbQ

Which happens in frontend/src/services/hooks/useGroups.js

Describe the solution you'd like
We have verified that there is no risk in this security finding, as the string replacement does not filter any user input (potential malicious input). In any case, we can fix it by using a regex, so it is better to have it fixed than to ignore it

P.S. Don't attach files. Please, prefer add code snippets directly in the message body.
@dlpzx dlpzx added type: enhancement Feature enhacement priority: high status: not-picked-yet At the moment we have not picked this item. Anyone can pick it up labels Sep 7, 2023
@anmolsgandhi anmolsgandhi added this to the v2.1.0 milestone Sep 8, 2023
@dlpzx dlpzx self-assigned this Sep 13, 2023
@dlpzx dlpzx mentioned this issue Sep 13, 2023
Merged
dlpzx added a commit that referenced this issue Sep 13, 2023
@dlpzx
feat: add npmaudit and semgrep in github workflows. Fix dependabot on…
14ac196 
… merge package (#751)

### Feature or Bugfix
- Feature
- Bugfix

### Detail
- add npm-audit github workflow on PR
- add semgrep worflow on PR and on schedule
- ignore semgrep issues with explanation. Those to be fixed will be
fixed in #739 and
#738
- remove make security checks that uses safety library and rename it as
linting
- upgrade all pacakges, add package-lock and pin merge to version 2.1.1

### Relates

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@dlpzx dlpzx mentioned this issue Sep 14, 2023
Merged
@noah-paige noah-paige linked a pull request Sep 15, 2023 that will close this issue
Fix: Add global flag to replace and avoid scanning issues on incomplete-sanitization #762
Merged
dlpzx added a commit that referenced this issue Sep 19, 2023
@dlpzx
Fix: Add global flag to replace and avoid scanning issues on incomple…
6cb136c 
…te-sanitization (#762)

### Feature or Bugfix
- Refactoring

### Detail
Use `replaceAll` instead of `replace` command in parsing the custom SAML
groups. This way all appearances of `[` and `]` are replaced.
I did not want to modify the command more as it is touching the
integration with other IdPs.

### Relates
- #739 

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
- Is the input sanitized? ---> ⭐ exactly this is what this PR is trying
to improve.
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@anmolsgandhi anmolsgandhi removed the status: not-picked-yet At the moment we have not picked this item. Anyone can pick it up label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dlpzx dlpzx

Labels
effort: low priority: high type: enhancement Feature enhacement
Projects
None yet
Milestone
v2.1.0
2 participants
@dlpzx @anmolsgandhi