Repository navigation
Do not panic when routeRegexp.regexp is nil - #816
Closed
clydeeshun94 wants to merge 3 commits into
Closed
clydeeshun94 wants to merge 3 commits into
clydeeshun94 wants to merge 3 commits into
Conversation
routeRegexp.Match called regexp.MatchString without checking the compiled pointer. Treat a nil routeRegexp or a nil compiled regexp as no match instead of panicking. Fixes gorilla#749
|
thaJeztah
reviewed
Sep 18, 2026
|
|
||
| func Test_routeRegexp_NilRegexp(t *testing.T) { | ||
| req := newRequest("GET", "/") | ||
| r := &routeRegexp{regexpType: regexpTypePath} |
There was a problem hiding this comment.
Same question as I left on the other PR;
This is manually constructing the routeRegexp, which is a type that's non-exported and has a constructor;
Lines 31 to 41 in db9d1d0
I'm not sure if fixing this by adding guards is the right thing to do; the related ticket indicates that either something is broken elsewhere, or that the code is used incorrectly, leading to the panic (which may actually be the correct thing to do for such cases);
routeRegexp.Match called regexp.MatchString without checking the compiled pointer. Treat a nil routeRegexp or a nil compiled regexp as no match instead of panicking. Fixes gorilla#749 Comment added explaining rationale: a no-match response is preferable to crashing the server on an unexpected nil state, as this panic has been observed in production.
Instead of only guarding in Match (defense-in-depth), this commit adds a check in newRouteRegexp that returns an error when RegexpCompileFunc returns nil without error. This prevents a nil regexp from ever being created through normal construction paths. The Match guard is kept as defense-in-depth for edge cases like copyRouteRegexp or RegexpCompileFunc returning nil without error.
Author
|
Closing this as it overlaps with #810 (same fix by different author). Will look for a new contribution to this repo or another repo in the future. Thanks for the review @thaJeztah. |
Rameen07304
added a commit
to Rameen07304/repo-pilot
that referenced
this pull request
Sep 27, 2026
Paste a pull request URL, get the architecture delta between its two
ends. Almost everything it needed already existed and was pointed at the
wrong thing — the comparison, the rename resolution, the evidence, the
page. Two pieces were missing:
* Reading a pull request's two commits from GitHub.
* Ingesting at a commit. get_repo_tree and get_file_contents already
took a sha; only the use case assumed HEAD, which is what stopped a
pull request being read at both of its ends. One parameter.
It compares against the merge base GitHub reports, not the tip of the
base branch. Against the tip, every unrelated commit landed while the
pull request was open gets blamed on it, which is what makes structural
diffs useless on a busy repository.
The URL is parsed, not pattern-matched: that string decides which
repository the server fetches, and a loose match sends the server's
token to github.com.evil.com. Tested against that and four other
lookalikes. A bad URL never reaches GitHub, and the private-repo guard
runs before the pull request metadata is fetched, not just inside ingest.
Verified against gorilla/mux#816 end to end: 34 seconds for two ingests,
and it correctly reports NO structural change — the PR is a nil check
inside a function, no imports touched. That is the useful answer: a
reviewer learns the architecture is untouched without reading the diff.
Two defects found and fixed while testing:
* A pull request number that does not exist returned 500. It is a 404.
* The input first went in the diagram tab, where it sat behind "ingest
a repo first" — for a feature that needs no repo loaded, because it
ingests both ends itself. It lives beside the repo URL now, folded
away, since both are "point me at something on GitHub".
Also checked, and found nothing wrong: mobile layout (no horizontal
scroll, nav reflows), all six tabs, console errors on a fresh load, and
that the CSP change really does let the webfont through.
449 Python tests, 66 frontend.
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 #749
routeRegexp.Match called regexp.MatchString without checking the compiled pointer. Treat a nil routeRegexp or a nil compiled regexp as no match instead of panicking.