Redirect directory requests missing trailing slash#252
Open
vibhor-aggr wants to merge 3 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 10 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements automatic trailing-slash redirect for directory requests in 🚥 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. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideAdds a guarded pre-send redirect for directory requests missing a trailing slash, including supporting utilities and tests, while documenting the new Sequence diagram for pre-send directory redirect in koa-static middlewaresequenceDiagram
actor Client
participant KoaApp
participant serve
participant redirectToDirectorySlash
participant send
Client->>KoaApp: HTTP GET /dir
KoaApp->>serve: invoke middleware
serve->>redirectToDirectorySlash: redirectToDirectorySlash(ctx, opts)
alt redirect performed
redirectToDirectorySlash-->>serve: true
serve-->>KoaApp: return (no send)
KoaApp-->>Client: 301/302 Location /dir/
else no redirect
redirectToDirectorySlash-->>serve: false
serve->>send: send(ctx, ctx.path, opts)
send-->>serve: done
serve-->>KoaApp: return
KoaApp-->>Client: static file response
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the
deferpath,redirectToDirectorySlashis called for all methods, so POST/PUT requests to a directory could now result in a redirect; consider mirroring the existing GET/HEAD guard insideredirectToDirectorySlash(or before calling it) to avoid changing semantics for non-idempotent methods. - The redirect URL is constructed with
ctx.path + '/' + ctx.search; it may be safer to normalizectx.search(e.g., ensure it is an empty string or already includes a leading?) to avoid accidental'undefined'or double-?issues across different Koa versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `defer` path, `redirectToDirectorySlash` is called for all methods, so POST/PUT requests to a directory could now result in a redirect; consider mirroring the existing GET/HEAD guard inside `redirectToDirectorySlash` (or before calling it) to avoid changing semantics for non-idempotent methods.
- The redirect URL is constructed with `ctx.path + '/' + ctx.search`; it may be safer to normalize `ctx.search` (e.g., ensure it is an empty string or already includes a leading `?`) to avoid accidental `'undefined'` or double-`?` issues across different Koa versions.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/index.js (1)
254-265: ⚡ Quick winAdd deferred-mode negative case for directory without index.
Lines 254-265 validate the redirect success path, but
defer: trueis still missing the mirrored “no index file => no redirect/404” assertion already covered in Lines 134-142 for non-deferred mode. Add that companion test to cover both branches in deferred flow.Suggested test addition
+ it('should not redirect when the directory has no index file', function (done) { + const app = new Koa() + + app.use(serve('test/fixtures', { + defer: true + })) + + request(app.listen()) + .get('/empty') + .expect(404, done) + })🤖 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 `@test/index.js` around lines 254 - 265, Add a companion deferred-mode negative test mirroring the non-deferred case at lines 134-142: create an it(...) named like "does not redirect directory requests missing a trailing slash in deferred mode when no index file", use app.use(serve('test/fixtures', { defer: true })) (same fixtures used by the non-deferred test), request the directory path used in the non-deferred negative test (e.g., '/noindex' or the same directory name from lines 134-142) with query params, and assert that the response is a 404 (or no redirect) and that there is no Location header (use request(app.listen()).get(...).expect(404) and an assertion that res.headers.location is undefined). Ensure the new test sits alongside the existing deferred redirect test so both branches are covered.index.js (1)
128-128: 💤 Low valueReplace deprecated
substrwithsubstringorslice.
String.prototype.substris deprecated. Usesubstringorsliceinstead for the same behavior.♻️ Suggested fix
function resolveFromRoot (root, pathname) { - const filename = pathname.substr(path.parse(pathname).root.length) + const filename = pathname.substring(path.parse(pathname).root.length) const resolved = path.resolve(root, filename)🤖 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 `@index.js` at line 128, Replace the deprecated String.prototype.substr call used to compute filename: instead of pathname.substr(path.parse(pathname).root.length) use a non-deprecated method (e.g., pathname.substring(path.parse(pathname).root.length) or pathname.slice(path.parse(pathname).root.length)) so that the filename calculation (variable filename derived from pathname and path.parse(pathname).root.length) behaves identically without using substr.
🤖 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.
Nitpick comments:
In `@index.js`:
- Line 128: Replace the deprecated String.prototype.substr call used to compute
filename: instead of pathname.substr(path.parse(pathname).root.length) use a
non-deprecated method (e.g.,
pathname.substring(path.parse(pathname).root.length) or
pathname.slice(path.parse(pathname).root.length)) so that the filename
calculation (variable filename derived from pathname and
path.parse(pathname).root.length) behaves identically without using substr.
In `@test/index.js`:
- Around line 254-265: Add a companion deferred-mode negative test mirroring the
non-deferred case at lines 134-142: create an it(...) named like "does not
redirect directory requests missing a trailing slash in deferred mode when no
index file", use app.use(serve('test/fixtures', { defer: true })) (same fixtures
used by the non-deferred test), request the directory path used in the
non-deferred negative test (e.g., '/noindex' or the same directory name from
lines 134-142) with query params, and assert that the response is a 404 (or no
redirect) and that there is no Location header (use
request(app.listen()).get(...).expect(404) and an assertion that
res.headers.location is undefined). Ensure the new test sits alongside the
existing deferred redirect test so both branches are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b35caeec-c1b2-468c-8d08-b5ae064a592d
📒 Files selected for processing (4)
Readme.mdindex.jstest/fixtures/empty/hello.txttest/index.js
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
1 participant
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 #130.
This adds a guarded directory redirect before delegating to
koa-send: when a GET/HEAD request targets a directory without a trailing slash and the configured index file exists,koa-staticredirects to the slash-appended URL while preserving the query string.The change keeps the existing explicit options intact:
format: truestill serves/dirdirectly.format: falsestill disables directory index formatting.Verification:
npm testnpm run lintgit diff --checkSummary by Sourcery
Handle directory requests without trailing slashes by redirecting to the slash-appended URL when an index file exists, while preserving existing directory handling modes.
New Features:
Documentation:
formatoption behavior for directory request handling modes.Tests:
defermodes, including cases without an index file.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests