fix: validate github return url metadata#5057
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds fail-fast validation and explicit errors for required metadata (installation_id, repository_id, pull_request_id/merge_request_id), wraps known GitHub API errors, validates repository owner/name and pull request HTMLURL, and derives installation_id from repository_id when missing. ChangesInput Validation and Error Handling for Metadata Fields
Sequence DiagramsNo sequence diagrams are generated for this PR because the changes are primarily validation, error-wrapping, and minor control-flow refactoring; the flows are simple and already documented in the hidden artifact. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
lukaszgryglicki
left a comment
lukaszgryglicki
left a comment
There was a problem hiding this comment.
/lgtm - will need rebase because I've fixed some failing CI in another PR. Once that PR is fixed this should became green.
Thanks! I’ll rebase the branch once the CI fix PR is merged. |
|
Yeah, it needs approval: #5066. |
|
Also this feedback from AI, PTAL: One correctness issue remains before merge: the changed Please align this function with the existing service method logic already present in the same file: read Small related fix:
At that point |
|
@lukaszgryglicki thanks. I’ll update it to derive the installation ID from "repository_id" like the existing service logic does, and I’ll fix the nil error case in "getInstallationIDFromRepositoryID" as well. |
|
Thanks, I've merged my PR, but wait a bit with rebasing because there might be a follow up PR... |
Got it, thanks for the heads up. I’ll wait before rebasing. |
|
Please rebase to newest |
psrsingh
force-pushed
the
fix-github-return-url-validation
branch
from
f16ffde to
50b6f0e
Compare
![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
🤖 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 `@cla-backend-go/v2/sign/service.go`:
- Around line 1572-1584: The metadata key checks currently accept empty strings
for repository_id, merge_request_id, and pull_request_id; update the checks in
the sign handler (the block that sets repositoryID, mergeRequestID,
pullRequestID from metadata) to reject empty values by using
strings.TrimSpace(value) == "" as a failure condition, construct the same error
(e.g., errors.New("missing repository_id in metadata") etc.), and call
log.WithFields(f).WithError(err).Warnf("unable to get ... for user: %s", userID)
then return the error just like the missing-key path so empty values are treated
as missing and do not produce malformed callback URLs.
- Around line 2242-2264: The package-level metadata parsing currently assumes
int/int64 types for pull_request_id, installation_id and repository_id and
rejects string/generic metadata; update this helper to use the same parsing
logic as (*service).getActiveSignatureReturnURL: accept string/generic values,
coerce with strconv (or fmt.Sprintf then strconv.ParseInt) for pull_request_id
and repository_id, derive installation_id from repository_id when appropriate
instead of requiring installation_id, and remove the strict int64 type
assertions so valid metadata processed by getActiveSignatureReturnURL isn't
rejected; keep log/error paths but parse values the same way the service method
does.
🪄 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: c377cdea-7a99-438f-8a76-975fc879932a
📒 Files selected for processing (2)
cla-backend-go/github/github_repository.gocla-backend-go/v2/sign/service.go
Rebased onto the latest |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cla-backend-go/v2/sign/service.go`:
- Around line 2299-2327: The service method (s *service)
getInstallationIDFromRepositoryID currently returns (0, nil) when
githubOrg.OrganizationInstallationID == 0; update this method to mirror the
package-level getInstallationIDFromRepositoryID behavior by creating and
returning an explicit error (e.g., errors.New("missing installation ID for
repository ID in metadata")), logging it with the same log fields and WithError,
and returning (0, err) so callers like getIndividualSignatureCallbackURL receive
a non-nil error instead of nil.
- Around line 1572-1591: The code calls metadataStringValue in sign/service.go
(used to extract repositoryID and mergeRequestID) but no such helper exists; add
a metadataStringValue helper in the sign package (or replace these calls with
the correct existing helper) that accepts the same metadata type used in this
file, returns (string, error) and performs the trim/empty check and error
creation (used by repositoryID and mergeRequestID extraction), then update
imports/visibility so sign/service.go compiles using that function name.
🪄 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: 11baa223-8e04-428f-8ed2-3995c38dba6b
📒 Files selected for processing (1)
cla-backend-go/v2/sign/service.go
|
Please take a look at the newest coderabbit review, also is this PR a fix for a specific issue? If so, which one? Becaus eit touches return URL functionality in a way that requires more IDs to be set in active signature metadata and might potentially break workign flows. So the question is why do we need this? CC @mlehotskylf - PTAL on this PR please. AI feedback: Two correctness items are still valid:
This PR calls Minimal fix: either add After that, the repeated
The new package-level Minimal fix in the service method: |
|
Looks like I know why CI fails - it is because the PR is from fork and forks don't get GitHub repo's secrets populated and some of repos used by go dependencies are private, so build/fetching fails and CI is red. Investigating/Researching this. |
That makes sense — thanks for digging into it. The error does line up with private dependency access failing from forked PR workflows, especially since GitHub Actions does not expose repository secrets to forks by default. So this likely isn’t related to the SSS changes themselves. I’ll continue cleaning up the remaining review feedback on my side while you investigate the CI setup/dependency access path. PTAL once the workflow situation is sorted. |
|
Thanks, I'm thinking about adjusting CI for forks, so it just red-fail. After initial research I can see that without making |
I applied the requested changes in Mainly:
I kept the changes minimal and did not touch dependency/module setup since the current CI issue seems unrelated to the PR changes themselves. |
|
Once this is merged #5073 - CI should be green for forks after rebasing. |
|
You can rebase the PR now, thanks, LMK when I can re-review it. |
lukaszgryglicki
left a comment
lukaszgryglicki
left a comment
There was a problem hiding this comment.
Needs rebase and updates after #5058 was merged.
Signed-off-by: psrsingh <psr.singh336@gmail.com>
Signed-off-by: psrsingh <psr.singh336@gmail.com>
psrsingh
force-pushed
the
fix-github-return-url-validation
branch
from
f6aee7b to
b009a3a
Compare
Rebased on latest dev and force-pushed the updated branch. There was a merge conflict in v2/sign/service.go, but it’s resolved now. |
|
OK, is this ready for review? |
Yep, it’s ready for review now. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cla-backend-go/v2/sign/service.go (1)
1588-1603:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
metadataStringValuefor callback metadata extraction to reject empty values consistently.These blocks still use raw
.(string)assertions, so""passes and can propagate malformed callback URLs. ReusemetadataStringValuehere to enforce trim/empty/"<nil>"checks uniformly.Proposed minimal fix
func (s *service) getIndividualSignatureCallbackURLGitlab(ctx context.Context, userID string, metadata map[string]interface{}) (string, error) { @@ - if found, ok := metadata["repository_id"].(string); ok { - repositoryID = found - } else { - err = errors.New("missing repository_id in metadata") + repositoryID, err = metadataStringValue(metadata, "repository_id") + if err != nil { log.WithFields(f).WithError(err).Warnf("unable to get repository ID for user: %s", userID) return "", err } - if found, ok := metadata["merge_request_id"].(string); ok { - mergeRequestID = found - } else { - err = errors.New("missing merge_request_id in metadata") + mergeRequestID, err = metadataStringValue(metadata, "merge_request_id") + if err != nil { log.WithFields(f).WithError(err).Warnf("unable to get merge request ID for user: %s", userID) return "", err } @@ func (s *service) getIndividualSignatureCallbackURL(ctx context.Context, userID string, metadata map[string]interface{}) (string, error) { @@ - if found, ok := metadata["repository_id"].(string); ok { - repositoryID = found - } else { - err = errors.New("missing repository_id in metadata") + repositoryID, err = metadataStringValue(metadata, "repository_id") + if err != nil { log.WithFields(f).WithError(err).Warnf("unable to get repository ID for user: %s", userID) return "", err } @@ - if found, ok := metadata["pull_request_id"].(string); ok { - pullRequestID = found - } else { - err = errors.New("missing pull_request_id in metadata") + pullRequestID, err = metadataStringValue(metadata, "pull_request_id") + if err != nil { log.WithFields(f).WithError(err).Warnf("unable to get pull request ID for user: %s", userID) return "", err }Also applies to: 1650-1667
🤖 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 `@cla-backend-go/v2/sign/service.go` around lines 1588 - 1603, Replace the raw type assertions for repository_id and merge_request_id with the shared helper metadataStringValue so empty/whitespace/"<nil>" values are rejected consistently: call metadataStringValue(metadata, "repository_id") and assign to repositoryID (handle returned error like the existing branches), and likewise call metadataStringValue(metadata, "merge_request_id") to set mergeRequestID; update the error handling/logging to use the helper's error and keep the same log.WithFields(f).WithError(err).Warnf("unable to get ... for user: %s", userID) pattern (affecting the blocks that set repositoryID and mergeRequestID in service.go, and the similar block around the other occurrence noted).
🤖 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.
Duplicate comments:
In `@cla-backend-go/v2/sign/service.go`:
- Around line 1588-1603: Replace the raw type assertions for repository_id and
merge_request_id with the shared helper metadataStringValue so
empty/whitespace/"<nil>" values are rejected consistently: call
metadataStringValue(metadata, "repository_id") and assign to repositoryID
(handle returned error like the existing branches), and likewise call
metadataStringValue(metadata, "merge_request_id") to set mergeRequestID; update
the error handling/logging to use the helper's error and keep the same
log.WithFields(f).WithError(err).Warnf("unable to get ... for user: %s", userID)
pattern (affecting the blocks that set repositoryID and mergeRequestID in
service.go, and the similar block around the other occurrence noted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f29c5bbf-2416-49cb-94bf-8281aeaaa021
📒 Files selected for processing (2)
cla-backend-go/github/github_repository.gocla-backend-go/v2/sign/service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cla-backend-go/github/github_repository.go
lukaszgryglicki
left a comment
lukaszgryglicki
left a comment
There was a problem hiding this comment.
-
metadataStringValue(service.go:1554) is declared but never called — the four sites atservice.go:1588, 1596, 1650, 1660still use the oldmetadata["..."].(string)assertion. Wire the helper into those sites, or drop the helper. -
getInstallationIDFromRepositoryIDerror wording: revert- err = fmt.Errorf("installation ID missing for repository ID: %s", repositoryID) + err = fmt.Errorf("missing installation ID for repository ID in metadata: %s", repositoryID)
PR #5058 just landed this line; rewording it is churn, and "in metadata" is misleading (value is missing from the GitHub org record, not metadata).
-
GetReturnURL— drop the redundant inner nil checks.go-github'srepo.GetOwner().GetLogin()andrepo.GetName()(andpullRequest.GetHTMLURL()) are nil-safe and return""on any nil in the chain, so theowner == "" || name == ""(resp. empty-HTML-URL) check already covers everything. Keep onlyif repo == nil/if pullRequest == nilplus the empty-string checks.
Signed-off-by: psrsingh <psr.singh336@gmail.com>
|
@lukaszgryglicki Addressed the review comments and pushed the updates. |
|
Lint failed - I will fix this small issue in post-merge. |
Thanks a lot for the help and reviews, really appreciate it! |
|
The problem is that |
Understood, makes sense. |
|
This is now deployed to |
Thanks! I'll test it and let you know if I find any issues. |
2 participants
Summary
Adds defensive validation for GitHub and GitLab return URL generation in the sign flow.
Changes
Why
The sign/auth redirect flow could continue with incomplete or malformed metadata, potentially resulting in invalid or empty redirect URLs and blank browser pages after authentication flows.
This patch improves robustness and error visibility around redirect URL generation.
Fixes #5052