Harden unsafe controller render defaults

[jamesfredley]

Description

What was found

Several controller render paths used browser-interpreted defaults for output that is not guaranteed to be safe HTML:

• render(Object) and unrecognized render(Map) values could produce inspect-style output without forcing a plain-text response type.
• render(file: ...) responses could be displayed inline unless the caller explicitly supplied a safer Content-Disposition.
• Generated Content-Disposition filenames did not escape unsafe quoted-string characters consistently.

What changed

• Default inspect-style object and unrecognized map rendering to text/plain when no explicit content type or response.contentType is set.
• Default file rendering to Content-Disposition: attachment.
• Preserve explicit caller-provided disposition headers and add inline: true for intentional inline file rendering.
• Escape unsafe filename characters in generated attachment headers.
• Document the new defaults and opt-outs in the controller render reference and Grails 8 upgrade guide.

Verification

• ./gradlew :grails-controllers:test --tests "grails.artefact.controller.support.ResponseRendererSpec" :grails-test-suite-uber:test --tests "org.grails.web.servlet.RenderMethodTests" :grails-doc:publishGuide -x aggregateGroovydoc
• Latest maintenance pass merged current 8.0.x; the only conflict was the upgrade-guide section ordering, resolved by preserving both the render-hardening note and the new upstream @EnableWebMvc / bean-overriding notes.

Open discussion

Maintainer feedback agrees with the filename/disposition hardening and asks for more discussion on the default content-type change. I left the branch intact so both protections can be reviewed together, but the filename/disposition part can be split into a narrower PR if maintainers prefer to defer the content-type default.

Contributor Checklist

Issue and Scope

• This PR has no acknowledged issue yet; the background above explains why the hardening is necessary for Grails 8 default behavior.
• This PR addresses the complete scope described above.
• This PR contains a single focused controller-render hardening change.
• This PR targets 8.0.x, where breaking default changes are permitted for discussion.

Code Quality

• I have added or updated tests that cover the changes introduced in this PR.
• I ran the focused verification listed above. A previous full combined run completed with 0 failures.
• The change follows the project's code style and avoids mass reformatting.
• Generative AI tooling was used as an ai-generated starting point and reviewed before submission.

Licensing and Attribution

• All contributed code is provided under the Apache License 2.0.
• I have the necessary rights to submit this contribution and confirm it is my own original work.
• Generative AI tooling use is labeled on the PR.

Documentation

• User-facing behavior is documented in the render reference and Grails 8 upgrade guide.
• The PR description explains what changed and why.

[Copilot]

Pull request overview

This pull request hardens Grails controller render(...) defaults to reduce unsafe browser-rendered responses by defaulting inspect-style renders to text/plain and making file responses download as attachments unless explicitly requested inline.

Changes:

• Default render(Object) and unrecognized render(Map) output to text/plain.
• Default file renders to Content-Disposition: attachment, with inline: true opt-out and preservation of explicitly set Content-Disposition.
• Escape unsafe characters in generated Content-Disposition filenames and document the updated defaults.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy	Implements hardened defaults (text/plain for inspect renders; attachment-by-default for files; filename escaping).
grails-controllers/src/test/groovy/grails/artefact/controller/support/ResponseRendererSpec.groovy	Adds focused unit coverage for the new render defaults and header escaping behavior.
grails-test-suite-uber/src/test/groovy/org/grails/web/servlet/RenderMethodTests.groovy	Updates/extends integration tests to validate attachment-by-default and inline opt-out for stream renders.
grails-doc/src/en/ref/Controllers/render.adoc	Documents the new default behaviors and the inline option for file rendering.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.3861%. Comparing base (30979e7) to head (4b01007).
⚠️ Report is 175 commits behind head on 8.0.x.

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #15801        +/-   ##
==================================================
- Coverage     49.3979%   49.3861%   -0.0118%     
+ Complexity      16814      16812         -2     
==================================================
  Files            1993       1993                
  Lines           93504      93504                
  Branches        16362      16362                
==================================================
- Hits            46189      46178        -11     
- Misses          40179      40190        +11     
  Partials         7136       7136                

see 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jdaugherty]

If the content type is unknown, why are we rendering something else?

[jamesfredley]

I looked at narrowing the PR to only the filename/disposition fixes and leaving inspect-style render output on the existing default, but review-gate flagged that as reintroducing the exact browser-interpreted inspect-output risk this branch is trying to avoid. The current implementation still renders the same inspect string; it only changes the default response media type for the inspect fallback from browser-interpreted HTML to text/plain unless the caller explicitly sets a content type. I am leaving this thread unresolved for maintainer discussion rather than treating it as resolved.

[jdaugherty]

Let's discuss in the weekly?

[bito-code-review]

The change ensures that when the content type is unknown, the system defaults to 'text/plain' instead of 'text/html'. This is achieved by updating the applyContentType method to accept a defaultContentType parameter, which is then passed to resolveContentTypeBySourceType when the content type cannot be determined.

grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy

private boolean applyContentType(HttpServletResponse response, Map argMap, Object renderArgument, boolean useDefault, String defaultContentType) {
        boolean contentTypeIsDefault = true
        String contentType = resolveContentTypeBySourceType(renderArgument, useDefault ? defaultContentType : null)

[jdaugherty]

I think to change these defaults we need to adhere to our PR summary policy and discuss the default change more thoroughly after a case has been presented.

[jdaugherty]

I'm not sure we should use a default content type. Especially plain text when encoders could cause non-text values to render.

[jdaugherty]

I 100% agree on the filename fixes. The default content type, I'd like to discuss more.

[jamesfredley]

Thanks. I updated the PR body to follow the summary policy more explicitly: it now separates "What was found", "What was fixed", and "Verification".

I agree the default behavior change needs maintainer discussion. The intent here is not to render a different semantic object, but to avoid browser HTML interpretation for inspect-style or otherwise unrecognized render output by forcing text/plain, and to make file renders download by default unless inline: true is explicit. I am leaving the default-change discussion open rather than marking it resolved.

[jamesfredley]

Pre-release review pass (2026-07-02, dual-reviewer: GPT-5.5 oracle + Codex CLI): NEEDS-IMPROVEMENT, now addressed.

Findings: the behavior-breaking render defaults were documented only in the render reference, not the upgrade guide (med), and the controller-level content-type contract was not asserted in RenderMethodTests (low). Both fixed in 4fb4967c08 - see the "Review follow-up" section in the description. One reviewer also caught an invalid render(myObject, contentType: ...) example in the first draft of the docs; the shipped example uses the valid render(text: ..., contentType: ...) form.

[jamesfredley]

Maintenance update: merged current 8.0.x into this branch and pushed the result. The only manual conflict was in grails-doc/src/en/guide/upgrading/upgrading80x.adoc; I kept the render-hardening upgrade note and the new upstream @EnableWebMvc / bean-overriding sections, renumbering the later sections.

I also reformatted the PR body around the repository checklist and left the filename/content-type split point explicit: the filename/disposition hardening can be split into a narrower PR if maintainers want to defer discussion of the text/plain default.

Verification: ./gradlew :grails-controllers:test --tests "grails.artefact.controller.support.ResponseRendererSpec" :grails-test-suite-uber:test --tests "org.grails.web.servlet.RenderMethodTests" :grails-doc:publishGuide -x aggregateGroovydoc passed.

[jamesfredley]

Status update

Merged current 8.0.x and resolved the upgrade-guide section-number conflict (kept Boot MVC subsections under @EnableWebMvc as 32.3-32.5; framework-bean override notes as section 33; render hardening remains section 31).

Open discussion for weekly meeting

Maintainer feedback on the default content-type change for inspect-style render(object) / unrecognized map rendering is still open:

• jdaugherty: prefer not defaulting unknown content to text/plain when encoders may produce non-text values; asked to discuss in weekly.
• Filename / Content-Disposition: attachment hardening is the less controversial part of this PR.

Options for the weekly:

1. Keep both protections in this PR (current branch).
2. Split: land filename/disposition now; park content-type default for a follow-up after discussion.
3. Drop content-type default and only ship disposition/filename.

Leaving the branch intact so both protections can be reviewed together until that discussion.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 4b01007
▶️ Tests: 17332 executed
⚪️ Checks: 61/61 completed

---

Learn more about TestLens at testlens.app.

[jamesfredley]

Agreed - let's split this so the uncontested part isn't blocked on the debated one.

Two independent changes are bundled here:

1. Filename / content-disposition hardening - you said you 100% agree on the filename fixes. This is straightforward security hygiene and can proceed.
2. Changing the default content type - this is the part worth discussing. Your concern is valid: defaulting to text/plain when the type is unknown can render encoder-produced non-text values incorrectly, and changing a render default is a behavior change that should go through the PR-summary policy with a documented case + weekly discussion.

I'll separate the two: keep the filename/disposition hardening in this PR, and pull the default-content-type change out into its own proposal so we can make the case and discuss it in the weekly without holding up the agreed fixes.

For the record (pre-release review): this PR is scoped to unsafe controller render defaults and is explicitly not the broader "GSP encode-by-default" topic (Google Doc 2.4) - that remains separate work.