Fix EasyCLA outage dev#5039
Merged
Merged
Conversation
WalkthroughThis PR refactors GitHub organization approval checking to use public org membership instead of per-org API calls, strengthens DynamoDB type safety by operating on typed AttributeValue maps directly, and updates the dependency audit allowlist for new high-severity advisories. ChangesGitHub Organization Approval Refactoring
DynamoDB AttributeValue Type Safety
Dependency Audit Allowlist
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR retries the Python→Go parity fixes that previously caused a production outage by (1) preventing DynamoDB attribute-type corruption from numeric-string coercion and (2) restoring GitHub org-based approval checks to use the public-orgs endpoint the OAuth bot can access.
Changes:
- Avoid
store.InterfaceMapToItemnumeric-string coercion by building/updating DynamoDBtypes.AttributeValuemaps directly (and healing previously-corrupted project records on PUT). - Fix CCLA GitHub org approval by listing a user’s public orgs (
GET /users/<user>/orgs) instead of querying org memberships (403 for the bot), and add unit tests via a stub hook. - Extend the Yarn audit allowlist with additional advisory IDs and notes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cla-backend-legacy/internal/api/handlers.go | Builds/patches DynamoDB AV maps directly for project create/update and normalizes stale numeric-typed string fields. |
| cla-backend-legacy/internal/api/handlers_project_test.go | Adds unit tests for project AV normalization/healing logic. |
| cla-backend-legacy/internal/api/github_oauth.go | Stops round-tripping user items through InterfaceMapToItem to avoid numeric-string coercion; preserves prior JSON “wire shape” for user_github_id. |
| cla-backend-go/signatures/service.go | Replaces org-membership checks with public-org listing for GitHub org approval and adds a test stub hook. |
| cla-backend-go/signatures/service_test.go | Adds regression tests for GitHub org approval behavior and guards ListUserPublicOrgs empty-input handling. |
| cla-backend-go/github/github_org.go | Introduces ListUserPublicOrgs helper wrapping GET /users/<user>/orgs with pagination and input validation. |
| .yarn-audit-allowlist.json | Allowlists additional Yarn audit advisories with explanatory notes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.yarn-audit-allowlist.json (1)
29-35: Allowlist debt is growing rapidly acrossaxiosandbasic-ftp— consider scheduling a dependency refresh.After this PR, the allowlist contains 5 basic-ftp advisories (1116289, 1116454, 1116478, 1117083, 1117726) and 5 axios advisories (1116365, 1116473, 1117575, 1117590, 1117592). The axios prototype-pollution gadget chain (
1117590) is particularly impactful: whenObject.prototypehas been polluted by any co-dependency, an attacker can silently intercept and modify every JSON response or fully hijack the underlying HTTP transport, gaining access to request credentials, headers, and body.Each individual deferral was reasonable in isolation, but the accumulation now means the audit scanner is effectively suppressed for two critical transitive dependencies. Consider opening a dedicated tracking issue for a backend
yarn.lockdependency refresh targeting at minimumsimple-git >= 3.32.0and the latest patchedaxiosandbasic-ftpversions, with a hard expiry date.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.yarn-audit-allowlist.json around lines 29 - 35, Create a dedicated tracking issue to schedule a backend dependency refresh targeting simple-git >= 3.32.0 and the latest patched axios and basic-ftp releases, reference the specific advisory IDs listed (basic-ftp: 1116289, 1116454, 1116478, 1117083, 1117726; axios: 1116365, 1116473, 1117575, 1117590, 1117592; simple-git: 1117673), assign an owner, set a hard expiry date for removing these temporary allowlist entries, and include steps to update yarn.lock, run CI/tests, and then remove or tighten entries in .yarn-audit-allowlist.json once patches are verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.yarn-audit-allowlist.json:
- Line 34: Update the allowlist entry for key "1117673" to stop silently
bypassing the simple-git RCE advisory: replace the current free-form note with a
factual entry that names the CVE IDs (CVE-2026-6951 / CVE-2026-28291), records
the patched version constraint (>= 3.32.0), and includes an explicit tracking
ticket reference (e.g., tracking ticket `#XXXX`) so the upgrade is not forgotten;
edit the value for "1117673" in .yarn-audit-allowlist.json (the existing
"1117673" entry) to contain those items and a brief justification for a
temporary allowlist if the upgrade cannot be applied in this PR.
In `@cla-backend-legacy/internal/api/handlers_project_test.go`:
- Around line 67-86: The test
TestPutProjectV1_BuildsStringTypesForDigitOnlyValues contains a tautological
first block that constructs AttributeValueMemberS directly and should be removed
or replaced; either delete the direct construction/assertion (lines creating
`item` with &types.AttributeValueMemberS and the subsequent asserts) and instead
invoke the handler or the shared item-builder used by PostProjectV1/PutProjectV1
to produce an item from a digit-only input and assert those produced attributes
are AttributeValueMemberS, or if a handler-level call isn't feasible, rename the
test to reflect it only verifies normalization (e.g. referencing
normalizeProjectStringFields) and keep only the meaningful stale-record rewrite
assertions; ensure you reference
TestPutProjectV1_BuildsStringTypesForDigitOnlyValues,
normalizeProjectStringFields, and the handler/builder
(PostProjectV1/PutProjectV1 or shared builder) when making the change.
---
Nitpick comments:
In @.yarn-audit-allowlist.json:
- Around line 29-35: Create a dedicated tracking issue to schedule a backend
dependency refresh targeting simple-git >= 3.32.0 and the latest patched axios
and basic-ftp releases, reference the specific advisory IDs listed (basic-ftp:
1116289, 1116454, 1116478, 1117083, 1117726; axios: 1116365, 1116473, 1117575,
1117590, 1117592; simple-git: 1117673), assign an owner, set a hard expiry date
for removing these temporary allowlist entries, and include steps to update
yarn.lock, run CI/tests, and then remove or tighten entries in
.yarn-audit-allowlist.json once patches are verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e23cb12-98e7-4299-b5ba-45c8732e47b6
📒 Files selected for processing (7)
.yarn-audit-allowlist.jsoncla-backend-go/github/github_org.gocla-backend-go/signatures/service.gocla-backend-go/signatures/service_test.gocla-backend-legacy/internal/api/github_oauth.gocla-backend-legacy/internal/api/handlers.gocla-backend-legacy/internal/api/handlers_project_test.go
ahmedomosanya
approved these changes
May 7, 2026
mlehotskylf
approved these changes
May 7, 2026
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.
Fixes prod outage (which is already reverted, but we want to retry porting py->go) (
dev)