Skip to content

fix(change): reject non-canonical git change URIs - #797

Merged
behinddwalls merged 2 commits into
uber:mainfrom
DivyanshGarg380:DivyanshGarg380/git-changeid-canonical
Oct 7, 2026
Merged

behinddwalls merged 2 commits into
uber:mainfrom
DivyanshGarg380:DivyanshGarg380/git-changeid-canonical

Conversation

@DivyanshGarg380

Copy link
Copy Markdown
Contributor

Why?

ParseChangeID currently accepts Git change IDs that aren't in canonical form.

net/url normalizes or drops parts of the URI, so different strings can end up representing the same ChangeID. The parser also accepts empty segments in the repository path.

What?

Reject change IDs when:

  • The parsed ChangeID doesn't round-trip to the original input
  • The repository path contains an empty segment

Added tests for uppercase schemes, userinfo, query parameters, fragments, non-canonical percent-encoding, and empty repository segments.

Test Plan

go test ./platform/base/change/git/...

python .\tool\bazel --output_user_root=C:\bazel test //platform/base/change/git:all

git diff --check

Issue

Closes #796

@behinddwalls

Copy link
Copy Markdown
Collaborator

i think this is already done? can you rebase and check again

@behinddwalls behinddwalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment

@DivyanshGarg380
DivyanshGarg380 force-pushed the DivyanshGarg380/git-changeid-canonical branch from bf9ee05 to be31d5c Compare October 7, 2026 17:36
@DivyanshGarg380

Copy link
Copy Markdown
Contributor Author

Hey, rebased the branch and pushed the updated changes. I also checked that the canonicalization fix is still not present in main

@behinddwalls
behinddwalls enabled auto-merge October 7, 2026 21:08
@behinddwalls
behinddwalls added this pull request to the merge queue Oct 7, 2026
Merged via the queue into uber:main with commit 561518b Oct 7, 2026
16 checks passed

This branch was previously deployed

1 inactive deployment
stack-rebase — 5c7b77e3 Deployed Oct 7, 2026 by behinddwalls via Rebase Stack #584
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] reject non-canonical git change IDs

2 participants