Fix more vulns#4930
Merged
lukaszgryglicki merged 4 commits intodevfrom Mar 11, 2026
Merged
Conversation
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSimplified and harmonized logging and error messages across GitHub auth and utility modules; tightened exception handling and minor session handling changes; added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to address reported vulnerabilities by updating JavaScript dependencies/lockfiles and reducing sensitive detail in backend logging.
Changes:
- Updated Node dependency versions/lockfiles (including new
package-lock.json, plusyarn.lockupdates). - Upgraded
tests/restNewman dependency set and adjusted dependency pinning configuration. - Reduced detail in Python backend logs around OAuth/session/API error handling.
Reviewed changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Updates resolved URLs and bumps several transitive packages (e.g., minimatch/brace-expansion/validator) for vuln remediation. |
tests/rest/yarn.lock |
Large lockfile refresh for REST test tooling dependencies. |
tests/rest/package.json |
Bumps newman and changes dependency pinning mechanism (now overrides). |
package.json |
Adds direct dependencies (minimatch, validator) to address advisories. |
package-lock.json |
Introduces an npm lockfile at repo root. |
cla-backend/cla/utils.py |
Reduces log detail and tweaks error handling messages in utility helpers. |
cla-backend/cla/models/github_models.py |
Reduces log detail around sessions/OAuth and adjusts some error handling/log messages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cla-backend/cla/models/github_models.py`:
- Around line 1591-1598: The code logs when token is None but then proceeds to
create OAuth2Session and call GitHub; change the control flow in the block that
references token and fn so that if token is None you log the warning and
immediately return an error (e.g., {"error": "Missing authentication token"})
instead of creating OAuth2Session or calling oauth2.get; update the branch that
currently reads "if token is None:" to return early to avoid unauthenticated
calls to OAuth2Session and the "/user/emails" request.
- Around line 1434-1438: The current use of del session["github_oauth2_state"]
and del session["github_oauth2_token"] can raise KeyError and mask the original
auth failure; update the code around session and fn to clear those keys
defensively by using a safe removal approach (e.g., call session.pop for
"github_oauth2_state" and "github_oauth2_token" with a default/no-op fallback or
check membership before deleting) before logging the warning and raising the
falcon.HTTPError so missing keys won’t raise additional exceptions.
In `@cla-backend/cla/utils.py`:
- Around line 2053-2055: The except Exception is too broad when extracting the
pull request number; narrow it to only the expected parse failures (e.g., except
(ValueError, IndexError) or a specific ParseError you use) around the extraction
logic and log that error via cla.log.warning(f"{fn} - error - unable to extract
pull request number from message: {e}"); for any other unexpected exceptions
re-raise them so they don't get swallowed (i.e., use a specific except for parse
errors and then a bare raise in a general except to propagate unexpected
errors).
- Around line 1745-1746: The failure branch currently returns a set literal
because of the braced f-string; change that to return a dict with an "error" key
so callers get a consistent, structured response. Replace the line returning
{f"..."} with something like return {"error": f"Could not get user gitlab group
id: {organization_id} members"}, keeping the cla.log.warning(...) call as-is;
this ensures the error branch of the function (the code around cla.log.warning
and the subsequent return) returns a mapping instead of a set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b03fbb8b-587e-4c7d-9e4b-a219386b2a92
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsontests/rest/package-lock.jsonis excluded by!**/package-lock.jsontests/rest/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
cla-backend/cla/models/github_models.pycla-backend/cla/utils.pypackage.jsontests/rest/package.json
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cla-backend/cla/models/github_models.py`:
- Around line 400-402: The current broad except Exception around the GitHub
token exchange (the block referencing fn, cla.log.warning and raising
falcon.HTTPBadRequest) incorrectly maps all failures to a 400; change it to only
catch and handle expected token-validation errors (e.g., the specific
exception(s) raised by your token-exchange routine such as ValueError or the
requests/HTTP client exception used in that path), log the warning and raise
falcon.HTTPBadRequest only for those cases, and let any other exceptions
propagate (or re-raise them) so they surface as 5xx; keep the existing log
message and falcon.HTTPBadRequest behavior for the narrowed exception types and
do not swallow unexpected exceptions.
In `@cla-backend/cla/utils.py`:
- Around line 1745-1752: The current code in the GitLab org users fetch path
logs the error and returns [] (using status_code and organization_id), which
hides API failures; instead, stop returning an empty list and propagate an
explicit error: raise a descriptive exception (e.g., GitLabAPIError or re-raise
the caught exception) that includes organization_id and status_code so callers
can distinguish a failed API call from an empty group; update the
fetch-gitlab-org-users function (the block that references status_code,
organization_id, and cla.log.warning) to raise that error after logging, and
adjust callers to handle or bubble the exception accordingly.
- Around line 2059-2061: The handler that parses pull_request_message currently
calls pull_request_message.splitlines()[0] without guarding against an empty
string, causing an unhandled IndexError; update the code around where
pull_request_message is used (look for the pull_request_message variable and the
splitlines()[0] call) to either check if pull_request_message is truthy and has
at least one line before indexing or add IndexError to the existing except tuple
(currently catching ValueError, json.JSONDecodeError, AttributeError, KeyError)
so empty messages are handled gracefully and the existing cla.log.warning path
is taken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03c61a87-4162-4d16-b7e0-e3e4bd28390b
⛔ Files ignored due to path filters (1)
tests/rest/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
cla-backend/cla/models/github_models.pycla-backend/cla/utils.py
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cla-backend/cla/utils.py (1)
2068-2070:⚠️ Potential issue | 🟡 MinorRemove the unused exception variable and consider cleaning up exception types.
The static analysis correctly flags that
eis assigned but never used. Additionally,json.JSONDecodeErrorandKeyErrordon't appear applicable since this function doesn't perform JSON parsing or dict key access.🔧 Proposed fix
- except (ValueError, json.JSONDecodeError, AttributeError, KeyError) as e: + except (ValueError, AttributeError): cla.log.warning(f"{fn} - error - unable to extract pull request number from message, parse error occurred")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cla-backend/cla/utils.py` around lines 2068 - 2070, Remove the unused exception variable and tighten the exception tuple in the except clause that logs pull request parse failures: change the except (ValueError, json.JSONDecodeError, AttributeError, KeyError) as e: to a bare except for the relevant types (e.g., except (ValueError, AttributeError):) and drop the "as e" since the variable is never used; keep the existing cla.log.warning(...) call and fn reference unchanged to preserve the log message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cla-backend/cla/utils.py`:
- Around line 2068-2070: Remove the unused exception variable and tighten the
exception tuple in the except clause that logs pull request parse failures:
change the except (ValueError, json.JSONDecodeError, AttributeError, KeyError)
as e: to a bare except for the relevant types (e.g., except (ValueError,
AttributeError):) and drop the "as e" since the variable is never used; keep the
existing cla.log.warning(...) call and fn reference unchanged to preserve the
log message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35d5f929-2309-4ac7-8e7e-4fb4fe42ed0c
📒 Files selected for processing (1)
cla-backend/cla/utils.py
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
ahmedomosanya
approved these changes
Mar 11, 2026
lukaszgryglicki
deleted the
unicron-address-codeql-and-dependabot-vulns-2
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io
Assisted by OpenAI
Assisted by GitHub Copilot