Skip to content

Fix codebase review findings across routing, servlet, multipart and server - #55

Merged
ghosthack merged 12 commits into
masterfrom
claude/lucid-galileo-7mdk26
Sep 27, 2026
Merged

ghosthack merged 12 commits into
masterfrom
claude/lucid-galileo-7mdk26

Conversation

@ghosthack

Copy link
Copy Markdown
Owner

Fixes the findings from a full codebase review. Each fix has a regression test. The test count goes from 243 to 331, and mvn verify passes, including the javadoc build.

Security and robustness

  • Multipart:
    • Repeated field names took quadratic CPU: about 200k parts under the 10 MB limit cost 18.6 s. Values are now stored in lists.
    • A request with more than 1000 parts gets a 413 (MultipartParser.setMaxParts).
    • Parsing makes fewer copies of the body.
  • Redirects:
    • redirect() now rejects every control character, including TAB.
    • New redirectLocal() and Validation.isLocalPath() block open redirects such as //evil.com, /\evil.com and https://…. A target that isn't a local path gets a 400.
  • Servlet backend: an exception from an action is logged and answered with a 500 instead of reaching the container, whose error page could show a stack trace.
  • Log forging: the embedded server logs the raw path with control characters escaped, so %0A in a URL can't forge log lines.
  • ClassForName: a class is no longer initialized before its type is checked, LinkageErrors are wrapped, and the class name is trimmed.
  • Release workflow:
    • Permissions are scoped to the job, and checkout sets persist-credentials: false.
    • The GPG passphrase is passed as MAVEN_GPG_PASSPHRASE.
    • The GitHub release is created as a draft, since publishing to Central is manual.

Correctness

  • Servlet forwards: Servlet.service() saves and restores the thread-local Env. Before, a forward()/alias() back into turismo broke the outer action.
  • Argument binding:
    • InputStream arguments are bound after the others. Before, (InputStream in, String name) gave a 500.
    • Reading form fields after the raw body is a 400.
  • controller():
    • Override detection compares name and parameter types. Before, an overload in a subclass hid a parent route.
    • Registration is all-or-nothing.
    • Parameter names of abstract methods are read from the concrete override.
  • Routing:
    • Trailing slashes and empty segments now count everywhere, in both App and the servlet ListResolver.
    • Re-registering a pattern route replaces it, as it already did for exact routes.
    • OPTIONS is answered automatically with a 204 and Allow, and 405 Allow headers list OPTIONS.
  • Float/double arguments: NaN, Infinity, hex values and overflow get a 400.
  • Multipart:
    • A field name can hold several files (getFile, getFiles, getFileNames), kept separate from text fields.
    • Query-string parameters are kept on multipart requests.
    • Backslashes in filenames are kept.
  • Servlet charset: a request without a charset is decoded as UTF-8, matching the embedded server.
  • Servlet router drift:
    • The servlet resolvers now block encoded-slash paths, as App does.
    • Registration is thread-safe (concurrent collections).
    • HEAD, 405 and OPTIONS are handled the same way as in App.
  • Embedded server:
    • Status codes outside 200–599 are rejected.
    • HEAD responses carry the GET body's Content-Length.
  • Alias: the forward target is validated.

New API

  • Turismo.stream() / HttpContext.stream(): opt-in chunked streaming, embedded server only.
  • Turismo.redirectLocal(...) and Validation.isLocalPath(...).
  • Multipart: FilePart, MultipartRequest.getFile/getFiles/getFileNames, and MultipartParser.setMaxParts.
  • Env.restore(Env), plus protected hooks on MethodPathResolver.

Docs

  • README JSP example: the servlet is mapped to / instead of /*, which forwarded JSPs back into turismo and gave a 404.
  • ExtendedRoutesMap section.
  • Server timeouts, documented rather than set, because the JDK reads them from global system properties.
  • Streaming and open-redirect guidance.
  • All behavior changes are listed under Upgrading to 5.0.

Behavior changes to note

  • /users/:id no longer matches /users/42/.
  • 405 Allow headers now include OPTIONS.
  • On the embedded server, status() throws for codes outside 200–599.
  • The multipart filename escape \" is no longer decoded; %22 is.

Not changed

  • jsp() still forwards by path rather than through the named jsp servlet: a named forward keeps the request path, which Jasper would try to render instead of the JSP file.
  • The two routing engines are aligned, not merged into one.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V


Generated by Claude Code

- HttpContext.status rejects codes outside 200-599 (0, 101, 1000 broke
  the status line or the connection)
- HEAD responses carry the Content-Length of the buffered GET body
- Server logs the raw, control-char-escaped path on errors, so a
  decoded %0A can't forge log lines
- Opt-in streaming: HttpContext.stream() / Turismo.stream() send status
  and headers and return a chunked body stream; status/header changes
  afterwards throw IllegalStateException, and errors after streaming
  are logged and end the response
- Validation.validateLocation rejects all control characters; new
  Validation.isLocalPath and Turismo.redirectLocal guard against open
  redirects
- Tests for the above, plus redirect(307, ...) and CR/LF rejection
  through the embedded server

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
- Store text fields as lists (linear time for repeated names) and cap
  the number of parts (MultipartParser.setMaxParts, default 1000;
  exceeding it throws ContentTooLargeException, answered with 413).
- Keep file parts apart from text fields and support several files per
  name via Parametrizable.addFile, FilePart, MultipartRequest.getFile,
  getFiles and getFileNames. The first file still backs the legacy
  [contentType, fileName] parameter and byte[] attribute.
- Merge the wrapped request's (query string) parameters with body
  fields, query values first.
- Treat backslashes in quoted parameters literally and decode only
  %22, %0D and %0A, as browsers send them; document file names as
  untrusted.
- Parse over ByteArrayOutputStream's buffer instead of copying it and
  decode text fields in place; only file contents are copied.
- Add tests, including MultipartFilter 400/413 responses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
- release.yml: job-level contents: write only, checkout with
  persist-credentials: false, GPG passphrase via MAVEN_GPG_PASSPHRASE
  (read natively by maven-gpg-plugin 3.2.8), GitHub release created as
  a draft since Central publishing is manual
- README: open-redirect warning and redirectLocal, streaming responses,
  status range and HEAD Content-Length, JDK server timeout properties,
  updated release steps

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
- Bind InputStream arguments after all other arguments (declared order
  kept for the call), so form parameters can still be read; a form read
  after the raw body is taken is now a 400 RequestException instead of an
  IllegalStateException.
- Detect subclass overrides by name and parameter types, and never treat
  a package-private method from another package as overridden, so an
  overload no longer hides the superclass's route.
- Make controller() atomic: build and validate every route, then
  register them only if all succeed.
- Treat trailing and empty path segments as significant for pattern
  routes (split with -1), consistent with exact routes; :name and *
  no longer match an empty segment. Add PathPattern.split().
- Re-registering the same method and pattern replaces the old route in
  place (last wins, like exact routes), serialized for thread safety.
- Resolve parameter names of an abstract route method from its
  concrete implementation; precise error when unavailable.
- Answer OPTIONS automatically with 204 and an Allow header when no
  OPTIONS route matches; list OPTIONS in 405 Allow headers too.
- Reject NaN, Infinity, hex floats, type suffixes and overflow for
  double/float arguments (400).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
A target taken from the request that points off-site is a bad
request, not a server error. Also correct form()'s Javadoc, which
now fails with 400 after the raw body was taken.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
Match App's routing in the servlet-side resolvers:

- A request whose raw URI contains %2F is resolved against the raw
  path's segments, each percent-decoded on its own, instead of the
  container-decoded path info. /admin%2Fsecret no longer reaches an
  /admin/secret route; /files/:name matches /files/a%2Fb with name=a/b.
  HEAD fallback and 405 + Allow apply to these requests too. Custom
  MethodPathResolver subclasses get 404 for them unless they override
  the new resolveEncoded() hook.
- ListResolver and MapResolver use concurrent collections, so routes
  can be registered while requests are resolved.
- MapResolver rejects null paths and actions; ListResolver rejects a
  null method (such routes could never match).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
- Servlet.service saves the thread's Env and restores it afterwards
  (new Env.restore), so a forward/alias/jsp that re-enters the servlet
  no longer leaves the forwarding action without an Env.
- Exceptions thrown by an action (including ActionException) are
  logged via System.Logger and answered with 500, or rethrown as a
  ServletException when the response is already committed, instead of
  reaching the container's error page.
- Requests without a declared charset are decoded as UTF-8, like the
  embedded server.
- Alias.forward rejects targets not starting with '/' with an
  ActionException; Javadoc warns against building targets from request
  input and explains the servlet mapping jsp() needs.
- ClassForName trims the name, loads without initializing until the
  type check passes, and wraps LinkageErrors (failed static init,
  missing classes) in ClassForNameException.
- Tests for Servlet.service, Env.restore, the action behaviors and
  ClassForName edge cases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
The web.xml example mapped turismo to /*, which also matches JSP paths,
so jsp() forwarded back into turismo and got 404. Map to / instead and
explain the options. Add an ExtendedRoutesMap section (RoutesMap has no
string alias), a complete JSP example, missing imports, and the servlet
backend's error, charset and encoded-slash behavior.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
- Split servlet paths with PathPattern.split so trailing slashes are
  significant there too, as in App.
- An unalignable encoded path matched the root route after the split
  change; use an explicit no-match sentinel instead.
- Answer OPTIONS with 204 and Allow, and list OPTIONS in 405 Allow
  headers, as App does.
- List this round's behavior changes under Upgrading to 5.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qjn6ZdPkHxbqCyPby4Lu4V
@ghosthack
ghosthack merged commit 9c00963 into master Sep 27, 2026
6 checks passed
@ghosthack
ghosthack deleted the claude/lucid-galileo-7mdk26 branch September 27, 2026 14:14
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.

2 participants