Security and bug fixes from the generated-test-suite review - #1836
Merged
Merged
Conversation
Dancer2::Handler::File joined public_dir with the request path and then
asked only whether the result was a readable file -- never whether it was
still inside public_dir. Path::Tiny::path does not collapse '..', so the
joined path escaped and -f was happy with it:
/hello.txt 200 HELLO
/../secret.txt 200 SECRET
/../../../../../../../../etc/passwd 200 (real /etc/passwd)
There was no depth limit, since there was no containment check at all;
surplus '..' segments simply collapse at the filesystem root, so an
attacker needed no knowledge of where public_dir sits. Any file readable
by the server process was reachable.
Reaching this required an application that both enables the File route
handler and sets static_handler: 0. With the default static handler on,
App::to_app wraps the application in Plack::App::File, which makes its
own '..' check and answers 403 first, so a default-configuration
application was not exposed.
The 403 guard at the head of the handler was meant to be this check, but
tested Path::Tiny::stringify for undef, which it never returns, leaving
the branch unreachable. It is replaced with the containment check
send_file already makes -- $dir->realpath->subsumes($file_path) -- so
both file-serving paths in this distribution now agree. The check is
made after the -f test, which guarantees every directory component
exists so realpath cannot die, and leaves a request for a file that is
simply absent passing through as before rather than becoming a 403.
Note this also refuses a symlink inside public_dir that points outside
it, which send_file has always refused.
Also stop the default static handler warning on a NUL in the path. Its
file-existence condition handed the raw PATH_INFO to Path::Tiny, which
warned "Invalid \0 character in pathname" on every such request, putting
an attacker-supplied path in the log; Dancer2::Handler::File rejects the
same input outright. Such paths are now refused before Path::Tiny sees
them. The 404 the client receives is unchanged.
Reported as F4 and F5 in a review by Curtis "Ovid" Poe.
Dancer2::Handler::AutoPage refuses to serve a template out of the layout
directory as a page, but decided that from the spelling of the request
path (m{^/\Qlayout_dir\E/}). On a case-insensitive filesystem -- the
default on macOS (APFS) and Windows (NTFS) -- a differently-cased request
misses the guard while resolving to the very same file, so /layouts/main
was correctly refused while /Layouts/main returned 200 and rendered the
layout.
The path-prefix test is kept as a cheap fast path, but the decision is
now made by resolving the page on disk and asking whether it lies inside
the resolved layout directory, using the same containment approach
send_file makes ($dir->realpath->subsumes($file_path)). That closes the
general "same file reached by a different spelling" shape rather than
just this one spelling.
The resolved page directory is derived from $template->views, which
Dancer2::Core::Role::Template guarantees is absolute, rather than from
view_pathname: that method is engine-specific, and
Dancer2::Template::TemplateToolkit overrides it to return a bare template
name for TT2's own INCLUDE_PATH to resolve, so it does not name a
location on disk. pathname_exists has already confirmed the page
resolves to a real file by that point, so realpath cannot die.
Reported as F11 in a review by Curtis "Ovid" Poe.
Given a before hook that dies and an on_hook_exception handler that sets
a response and calls is_halted(1), the route the before hook had just
refused was executed anyway -- along with any side effect it has: a
charge, an insert, an email:
before -> hook_exception(core.app.before_request)
-> the route body runs
-> hook_exception(core.app.after_request)
The wrapper captured is_halted, then called cleanup, which clears the
request, the response and the session, and then returned without
croaking because the handler had halted. Dispatch resumed in
_dispatch_route, read $self->response -- by now a fresh, unhalted object
-- saw nothing halted, and ran the route. The 418 the client finally
received came from the second firing of the exception handler, triggered
when core.app.after_request died on the request cleanup had destroyed,
not from the first.
The wrapper's own comment states that halting in the handler is a
supported way to retain a custom response, so cleanup is now skipped when
the response was halted: the halt means this response is final, and the
state dispatch still has to read must survive for it to be returned.
Also make hook compilation idempotent. compile_hooks wraps each hook and
writes the wrappers back with replace_hook, so calling to_app again
wrapped the already-wrapped hooks, and on the failure path each layer
treated the inner layer's croak as a fresh hook failure and fired
core.app.hook_exception itself -- one dying before hook reporting 1, then
2, then 3 times as to_app was called again. compile_hooks now records
each wrapper it produces and passes its own earlier work through
untouched.
That is deliberately per-hook rather than a per-app "already compiled"
flag: finish() adds postponed plugin hooks immediately after calling
compile_hooks, so a flag would leave those permanently unwrapped on a
later to_app call -- and an unwrapped hook does not merely go unreported,
it dies through the dispatcher without ever reaching
core.app.hook_exception. A test covers that case.
Reported as F9 and F10 in a review by Curtis "Ovid" Poe.
headers_to_array applied two substitutions to every header value -- one
folding linear whitespace, one removing CR and LF, commented "remove CR
and LF since the char is invalid here" -- and applied nothing at all to
the header name pushed onto the same array two lines later. A CRLF
injected into a name therefore reached the PSGI header array intact:
$response->header("X-Bad\r\nInjected: yes" => 'v')
which is the same response-splitting shape the value is guarded against.
Whether it reaches the wire depends on the PSGI server; some validate
header names, many do not. The name now gets the same treatment as the
value, so no element of the array can carry CR or LF.
Also fix content assigned a second time never being encoded. is_encoded
latched on the first assignment and encode_content returns early whenever
it is set, so with charset UTF-8:
$response->content('first'); # encodes, latches is_encoded
$response->content("hi\x{263A}"); # returned untouched
left a wide character in the PSGI body, a Content-Length of 3 (the
character count) rather than the 5 bytes UTF-8 needs, and a Content-Type
still announcing charset=UTF-8. This is reachable from ordinary
application code: an after hook that rewrites response->content, or
halt() called once content had already been set.
is_encoded now describes the content currently assigned rather than the
response for the rest of its life: the 'around content' modifier clears
it before encoding, so each assignment is judged on its own merits. The
callers that mark already-read bytes by hand are unaffected --
Handler/File.pm sets is_encoded(1) after calling content(), and
send_file assigns through the raw hash element, bypassing the modifier
entirely. Neither sets it before assigning, which is the only order this
would break.
Reported as F6 and F7 in a review by Curtis "Ovid" Poe.
The lookup was an exact hash-key match on the raw header value, so a
perfectly ordinary charset parameter made it miss:
Content-Type: text/x-yaml 200, deserialized as YAML
Content-Type: text/x-yaml; charset=utf-8 400
The miss selected the JSON fallback, which was then handed a YAML body
and failed the request. The same happened on Accept, where
'text/x-yaml; charset=utf-8' silently returned JSON instead of YAML.
JSON escaped notice only by accident: the fallback when the lookup misses
is JSON, so 'application/json; charset=utf-8' still worked and hid the
defect for the commonest case.
The header is now normalised before the lookup -- parameters stripped at
the first ';', trimmed, and lowercased -- so the mapping is keyed by bare
content types, as its own documentation describes them, and an uppercase
spelling matches too. This is what the rest of the distribution already
does: Response.pm calls content_type_charset precisely to split the
charset off the type.
A comma-separated multi-type Accept header is unchanged: it is still
matched whole, not split, and so falls through to the default.
Also correct the DESCRIPTION, which gave one precedence order for both
directions. serialize() calls _get_content_type('accept') and
deserialize() calls it with 'content_type', so the orders genuinely
differ, and the code is right to differ: Accept is what a client uses to
say what it wants back, which need not be what it sent. The
documentation now describes both directions instead of contradicting one
of them.
Reported as F3 and F8 in a review by Curtis "Ovid" Poe.
For a route declared as get 'item' => '/item/:id', uri_for_route('item',
{ id => 7 }) and { id => 'abc' } both worked, but { id => 0 } died with
"Route item uses the parameter 'id', which was not provided" -- when it
had been provided. The empty string behaved the same way. The value was
tested for truth:
my $value = $route_params->{$param} or die ...
so any defined-but-false value was rejected, though 0 is an ordinary
database ID, list index or page number. The value is now tested for
definedness, and only a genuinely absent parameter dies.
Reported as F1 in a review by Curtis "Ovid" Poe.
The log_format documentation listed %D, described as "timer", but map_chars_to_subs returns no D key, so a logger configured with it -- log_format: '[%D] %m' -- warned "%D not supported." through Carp on every single message logged and rendered the field as '-'. Anyone who followed the documentation was getting a warning per log line. The formatting lives in the shared role, so this applied to every logger engine. %D is removed from the list rather than implemented: there is no timing state in the logger role to implement it from, and adding some is a feature rather than a fix for the warning. A cross-check of the whole documented list against map_chars_to_subs found no other mismatch in either direction. Also document what an unrecognised format character does, since that is what a user who mistypes one will hit, and cover the full documented list in the tests so this cannot drift again. Reported as F2 in a review by Curtis "Ovid" Poe.
`dancer2 gen -a G::App --path DIR -d gapp -g` wrote the whole application
and then died with "Can't locate object method "absolute" via package
"DIR/gapp"", exiting 255. The same happened for -r <uri>, which implies
-g. _check_git took its path from $vars->{apppath}, which run() had
stored as a plain string, and called ->absolute on it; the very next line
of run() keeps the Path::Tiny object for exactly this purpose, as
$vars->{appdir}. Since _check_git is called after the files have been
copied, the user was left with a generated application, no repository --
git init, git add, the initial commit and any git remote add all come
after the chdir that died -- and a stack-shaped error where the "Your new
application is ready" banner should have been. It now uses the absolute
path run() had already computed.
`dancer2 gen -a Other::App` with no -d created a directory named
"Other::App", colons and all, rather than "Other-App". _get_app_path
exists to produce the dashed spelling and was being called, but the
directory option defaulted to the raw application name and overrode it,
so a single generated application disagreed with itself: the directory
was Other::App while the Makefile.PL beside it cleaned Other-App-*. The
option now has no default and falls back to the dashed name. An explicit
-d is still honoured verbatim.
The line appended to a generated MANIFEST.SKIP was built from the full
filesystem path the application was generated into -- ^/tmp/xYz/myapp- --
so it could never match: ExtUtils::Manifest matches these patterns
against paths relative to the distribution root, and every other pattern
in the skeleton's MANIFEST.SKIP is relative. It now appends the dashed
distribution name already computed for Makefile.PL's cleanfiles.
Finally, rename share/.gitignore to share/gitignore. That file is
shipped data, copied into the user's new application by _check_git, and
its sessions/, logs/ and environments/ entries describe a running
Dancer2 application -- but living at that path made git honour it
against this distribution's own share/ tree as a side effect, where
'environments/' covers the skeleton's per-environment configs. The
generated application still receives it named .gitignore, which the copy
now states explicitly rather than leaving to the destination directory.
Reported as F12, F13, F14 and F15 in a review by Curtis "Ovid" Poe.
The remainder of a characterization suite written against this distribution by Curtis "Ovid" Poe, covering areas the preceding fixes did not touch: config reading, merging and strict mode; session lifecycle; error rendering and its fallback to static pages; dispatch flow; engine hooks; template rendering; cookies; routes; requests; and logger levels. These pin behaviour as it stands today rather than asserting anything new, so they document what the framework currently promises and turn a future change to it into a visible decision. The comments say why each thing is asserted, including where current behaviour is pinned without being endorsed. The XS query-parser subtest skips where CGI::Deurl::XS is not installed rather than dying, since it is an optional dependency.
Testing the parameter with defined alone let the empty string through, which is not the same case as 0. A ':param' compiles to ([^/]+) and so matches at least one character, meaning an empty value produces '/item/' -- a URI that 404s against the very route it was generated from. Handing back a URL that cannot work is worse than refusing to build one. The two rejections now have separate messages. Reusing "which was not provided" for an empty string repeated the original complaint about this code: it told the caller their parameter was missing when they had provided it, just with a value the router cannot use. 0 continues to be accepted, and still round-trips.
Both failed CI on the 5.36 job while passing locally, for reasons that had nothing to do with what they were meant to be testing. t/e2e/cli/gen.t asked git whether the skeleton's environment configs are tracked, guarded by --is-inside-work-tree. That guard is too weak for the way the suite actually runs under CI: 'dzil test' builds into .build/XXXX inside this checkout and runs from there, so the guard passes, while 'git ls-files <path>' -- whose pathspec is relative to the current directory -- names a build artifact rather than the source file and reports nothing tracked. It now resolves the top of the tree with --show-toplevel and queries from there, and skips when the top level is not this source tree or there is no git at all, as on a smoker unpacking a released tarball. t/unit/core/cookie.t compared to_header against the implementation the build selected as strings. HTTP::XSCookies::bake_cookie is handed a hashref and walks it to build the attribute list, so its output order is not stable -- two calls in a single process can return 'n=v; Path=/; HttpOnly' and 'n=v; HttpOnly; Path=/'. Asserting the string failed about half the time wherever HTTP::XSCookies was installed, which is why it passed here and failed there. RFC 6265 gives Set-Cookie attributes no significant order, so the comparison is now against the name=value pair plus the sorted set of attributes. Neither change weakens what the tests cover: the git one still asserts both files are tracked wherever that is knowable, and the cookie one still fails if to_header is aliased to neither implementation.
The comments and subtest names carried tags like "F14" and pointers to
paad/test-roadmap/test-roadmap-findings.md, both of which live in the
report this work came from rather than in this repository. A reader
coming to the code later has neither, so the tag was doing no work and
in a few places was the only thing carrying the reason for the code.
Replace them with the explanation itself. Where the tag stood in for a
description, spell the description out: the leading dot on
share/.gitignore made git apply its patterns to this distribution's own
share/ tree, which is how the skeleton's environments/ configs came to
be missing from the shipped dist; calling ->absolute on the plain string
in $vars->{apppath} is what made "dancer2 gen -g" die with "Can't locate
object method "absolute" via package ...".
Also add a newline to the two uri_for_route() parameter errors, so perl
does not append the file and line number. Both are raised at the
caller's mistake rather than at an internal fault, so the source
location is noise to whoever sees it, and a pentest would fairly read a
full source path in a response as a low-level information leak.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review flagged it as a likely typo, which is fair - it looks like one. It is perl's internal name for the file-test ops, and it reaches us verbatim in the warning Path::Tiny emits, so it is quoted here exactly rather than tidied. Say so in the comment, so the next reader does not have to have the same exchange to find out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The subtest asserted that a route returning a plain, non-reference string comes back as a 200 with the default text/html content type and an empty body: the JSON encoder refuses a top-level scalar, and the die is caught and logged rather than propagated, so the response is never given the serializer's content type. That is only true where the encoder refuses. RFC 8259 permits any value at the top level, unlike RFC 4627 before it, and Cpanel::JSON::XS changed to match: 4.37 dies on a plain scalar, 4.43 encodes it. So the same code answers 'application/json' with a body of '"plain string"' there, and CI failed on exactly that while passing locally -- the difference was the version of one module, not the branch. Probe the behaviour and assert against what was found. Naming the backend would be wrong twice over, since this varies by version within a single backend as well as between them, and Dancer2::Serializer::JSON passes no allow_nonref either way. Both outcomes stay asserted rather than skipped, so whichever the environment has is still covered, and the note() reports which one it was so a failure can be read without guessing. Neither outcome is endorsed as correct. Which of the two Dancer2 should give is a real question, but it belongs with the serializer rather than in a test that is only meant to pin what happens today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The native job installs the built tarball with 'cpanm Dancer2-*.tar.gz'.
When the test suite fails under that, cpanm reports only
Building and testing Dancer2-x.y.z ... FAIL
and writes the actual test output to its own build.log, which is not part
of the job output and is never uploaded. So a red macOS job currently
says that something failed and nothing whatsoever about what -- not the
test file, not the assertion, not the diagnostics.
Print the log on failure. It runs only when the job has already failed,
so a passing run is no noisier than before, and it is skipped on Windows,
which installs with --verbose and so has the output inline already.
The loop guards against the glob not matching, so the step cannot itself
fail on a runner that got as far as failing before cpanm wrote anything.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard added to stop a layout being served as a page resolved both
directories with realpath and compared the results with subsumes. That
does not work on the filesystems the guard exists for.
realpath resolves symlinks but does not canonicalise case, and
Path::Tiny::subsumes is a prefix match on the path string. So on a
case-insensitive filesystem - macOS and Windows by default - a request
for /Layouts/main gives
layout dir .../views/layouts
page dir .../views/Layouts
subsumes() false, though both name one directory, same inode
and the layout is rendered as a page. The previous code decided from the
spelling of the request; this decided from the spelling of the resolved
path. Same blind spot, one step later, and still absent exactly where the
bug was reachable: on a case-sensitive filesystem the differently-cased
request never found the file to begin with.
Compare device and inode instead, walking the page directory's ancestors
up to the views directory. No spelling can disguise identity, so this
settles case folding, symlinks and hardlinks together, and there is
nothing above views worth reasoning about.
Some Windows configurations report every inode as 0, which would make
every directory equal to every other. Where the inode is unusable the
comparison falls back to matching paths case-insensitively: it assumes the
folding rules rather than asking, but it fails closed on the case this
guard is about instead of silently admitting everything.
The containment check in Dancer2::Handler::File is not affected. There
the prefix is public_dir, fixed by configuration, and realpath collapses
the '..' segments before the comparison - a case difference there can only
refuse a request that should have been served, never admit one that
should not.
Verified both ways: on a case-insensitive filesystem (a loopback vfat
mount) the guard now fires and the request 404s, and the case-sensitive
run is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
perldoc -f stat: where an inode number is of a type larger than perl can hold as an integer, stat returns it as a decimal string to preserve the whole value. Comparing two of those numerically converts them to floats and rounds them, so two different inodes above 2**53 - reachable on large ZFS, XFS and btrfs volumes, and on some network mounts - can compare equal. For this guard that rounds in the wrong direction: a page directory that is not the layout directory would be taken for it, and the page served rather than refused. eq is exact, and is equally correct for the inode numbers that are returned numerically. Spotted by veryrusty in review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AYumxw4bfLvz6e4Cbjtrj8
Brings in the CI fixes, the app-root detection work, the Crypt::URandom session-ID change and the removal of the Dumper serializer from core. Two conflicts, both in files this branch also touched: Changes - both sides added entries to the same release block. Kept both; main's [ REMOVALS ] section follows our [ SECURITY ] one, and its PR #1804 bug-fix entry sits with the other upstream entries. Serializer/Mutable.pm - both sides rewrote overlapping parts of the POD. Our code change (normalising the header before the mapping lookup) and main's (gating Dumper behind enable_dumper) do not touch the same subs and merged cleanly; only the prose collided. Taken together rather than by side: our two-direction description of the header precedence, main's removal of Dumper from the mapping tables and its new Dumper section, and our dropped "the keys of the mapping are the content-types" clause, which the paragraph we added above it now states more precisely. t/integration/serializer/mutable.t needed a change the merge could not flag: it asserted that Accept: text/x-data-dumper serializes with Dumper, which is no longer in the default mapping. The case moves to the list of types that fall back to JSON, where it now pins something worth pinning - that an app which has not set enable_dumper does not reach the Dumper serializer, and so does not eval a request body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AYumxw4bfLvz6e4Cbjtrj8
Each entry described its bug in full because there was nowhere to point at: the work was done in a private repository and had no public tickets. Now that GH #1822-#1835 exist, the detail lives there and the changelog can say what changed in a line, matching the surrounding entries. The two entries covered by a security advisory cite the advisory rather than a ticket, since filing a public issue describing either one would disclose it as surely as publishing the advisory does. Also credits Curtis "Ovid" Poe in [ MISC ], whose generated test suite found the defects fixed here; the individual commits credit him too, but the changelog is what most people will read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AYumxw4bfLvz6e4Cbjtrj8
cromedome
approved these changes
Sep 16, 2026
cromedome
left a comment
cromedome
left a comment
Contributor
There was a problem hiding this comment.
Reiterating that @veryrusty and I already reviewed and approved in a private repo.
cromedome
added a commit
that referenced
this pull request
Sep 16, 2026
[ SECURITY ]
* PR #1836: Fix path traversal in Dancer2::Handler::File, which
served files from outside public_dir; see GHSA-6xw8-v24c-m783
(David Precious)
* PR #1836: A halting on_hook_exception handler no longer lets the
route the hook refused run anyway; see GHSA-v527-r4px-7vx7
(David Precious)
* GH #1822: Strip CR and LF from response header names, as was
already done for header values (David Precious)
* GH #1823: AutoPage no longer serves a layout as a page on
case-insensitive filesystems (David Precious)
[ BUG FIXES ]
* GH #1781: Fix directory detection heuristic (Jason A. Crome)
* GH #1784: Fix UTF-8 handling in Serializer::JSON for readonly
values (Russell @veryrusty Jenkins)
* GH #1790: Fix infinite recursion into blessed objects in JSON
Serializer (Russell @veryrusty Jenkins)
* PR #1791: Send correct error codes in send_file (Anton Lundin)
* PR #1797: Fix path()/dirname() DSL keywords dropping their first
argument (Mike Weisenborn)
* PR #1801: Fix failing CI (Jason A. Crome)
* PR #1804: Make session ID generation always use Crypt::URandom and
harden validate_id against invalid session IDs (David Precious)
* GH #1824: Encode each response content assignment on its own
merits, not just the first (David Precious)
* GH #1825: Serializer::Mutable now ignores content type parameters
such as charset when choosing a format (David Precious)
* GH #1826: uri_for_route accepts a route parameter of 0, and
refuses an empty one with a clearer message (David Precious)
* GH #1827: Hooks are compiled exactly once however many times
to_app is called (David Precious)
* GH #1828: A NUL byte in a static file request no longer warns once
per request (David Precious)
* GH #1829: dancer2 gen -g (and -r) no longer dies after writing the
application (David Precious)
* GH #1830: dancer2 gen names the application directory after the
dashed distribution name (David Precious)
* GH #1831: dancer2 gen appends a relative, matchable pattern to
MANIFEST.SKIP (David Precious)
[ ENHANCEMENTS ]
* None
[ DOCUMENTATION ]
* GH #1832: Document Serializer::Mutable's actual header precedence
in each direction (David Precious)
* GH #1833: Remove %D from the documented log_format characters; it
was never implemented (David Precious)
[ DEPRECATED ]
* PR #1821: Remove Data::Dumper serializer from Dancer2 core, along
with from_dumper/to_dumper keywords, tests (David Precious)
[ MISC ]
* GH #1834: Rename share/.gitignore so git stops applying it to this
distribution's own share/ tree (David Precious)
* GH #1835: Add a characterization test suite under t/unit,
t/integration and t/e2e (David Precious)
* With thanks to Curtis "Ovid" Poe, whose generated test suite from
PAAD for Dancer2 found the defects fixed above
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
2 participants
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.
Security and bug fixes arising from a review of Dancer2 against a generated test suite written by Curtis "Ovid" Poe. The work was done and reviewed in a private repository so the security issues could be fixed before disclosure; this is that branch, approved there by @cromedome and @veryrusty.
Security
Two of these are covered by published advisories:
Dancer2::Handler::Fileserved files from outsidepublic_dir. The request path was joined ontopublic_dirand then tested only for being a readable file, never for still being insidepublic_dir.Path::Tinydoes not collapse.., so/../secretescaped at unlimited depth. Reachable only by an application that enables theFileroute handler and setsstatic_handler: 0— the default static handler refuses such paths first.beforehook that refused a request could be overridden. Anon_hook_exceptionhandler that set a response and halted had that response destroyed by the cleanup call that followed, so dispatch read a fresh, unhalted response and ran the very route the hook had refused, side effects and all.Two more are lower severity and have ordinary tickets:
Fixes #1822
Fixes #1823
Bug fixes
Fixes #1824
Fixes #1825
Fixes #1826
Fixes #1827
Fixes #1828
Fixes #1829
Fixes #1830
Fixes #1831
Documentation
Fixes #1832
Fixes #1833
Misc
Fixes #1834
Fixes #1835
Notes for review
This branch has been reviewed in full already on a private fork (because it included security issues), so the useful thing to look at is what came after the approvals:
293fce08resolved both directories withrealpathand compared them withPath::Tiny::subsumes. Neither canonicalises case, so on a case-insensitive filesystemviews/Layoutsandviews/layoutscompared as two directories while naming one — the guard never fired, and the bug survived the fix on exactly the platforms where it was reachable. It passed CI only because the Linux jobs cannot reach the bug either way.753b3853compares device and inode instead, and7aa24256compares inode numbers witheqrather than==(perperldoc -f stat, a large inode comes back as a decimal string, and comparing numerically rounds it). Verified on a loopback vfat mount and on the macOS CI job; @cromedome separately confirmed the Windows path.Changesis deliberately terse. Each entry points at its ticket rather than describing the bug, since the tickets carry the detail.Please do not squash.