Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation for form limits & improve configuration via context attributes #12560

Open
wants to merge 4 commits into
base: jetty-12.0.x
Choose a base branch
from

Conversation

lachlan-roberts
Copy link
Contributor

  • Add documentation page for limiting form content.
  • FormFields.from already reads this configuration from context attributes, but this PR makes it so that those context attributes delegate to setting the equivalent fields on ContextHandler.
  • Add information about maxFormKeys and maxFormContentSize to the dump.

replaces #12232

@lachlan-roberts lachlan-roberts changed the title Add documentation for form limits & improve configuration via contet attributes Add documentation for form limits & improve configuration via context attributes Nov 22, 2024
joakime
joakime previously approved these changes Nov 22, 2024
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a similar security directory in the operations guide.

There, make a similar section titled "Limiting Form Content" (same title), that points to that of the programming guide for example:

Forms can be a vector for denial-of-service attacks, like explained in xref:...[this section].

Then proceed to explain operation-guide specific configuration.

We should have these form limits as a Jetty module properties, and if we don't already, we should add them.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could simplify the dump by just adding a bean to dump the extra info from a WebApp, rather than override dump again and repeat.

Perhaps a bit outside the scope of this PR?

Comment on lines 322 to 323
Dumpable.named("maxFormKeys ", getMaxFormKeys()),
Dumpable.named("maxFormContentSize ", getMaxFormContentSize()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move down in the dump to be after the attributes

@@ -987,6 +988,8 @@ else if (getBaseResource() != null)
name = String.format("%s@%x", name, hashCode());

dumpObjects(out, indent,
Dumpable.named("maxFormKeys ", getMaxFormKeys()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than override dump and have to repeat the stuff from the base class, why don't we just add all these Dumpable collections as beans the the context and let the normal dump mechanism dump them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better yet, add one Dumpable webapp bean, that dumps all these extra details

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a separate issue for this (see #12655).
As its outside the scope of this documentation PR.

Signed-off-by: Lachlan Roberts <[email protected]>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make it clear that the attributes apply to both core and webapp contexts. For core applications you should reference FormFields#onFielda and that the limits can be passed in there, else they are taken from context or server attributes.

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.

4 participants