feat: warn when json validator does not find a content type in the request

[Soviut]

When a validator has a target 'json' it will now warn if a request is missing a Content-Type: application/json header.

Closes honojs/middleware#841

Added some docs too honojs/website#540

The author should do the following, if applicable

• Add tests
• Run tests
• bun run format:fix && bun run lint:fix to format the code
• Add TSDoc/JSDoc to document the code

[yusukebe]

Thanks! I'll check it later.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.70%. Comparing base (190e122) to head (d83827f).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3707   +/-   ##
=======================================
  Coverage   91.70%   91.70%           
=======================================
  Files         159      159           
  Lines       10158    10164    +6     
  Branches     2855     2870   +15     
=======================================
+ Hits         9315     9321    +6     
  Misses        842      842           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yusukebe]

Hi @Soviut

Thank you for the PR!

Do you think that we should show a similar message for form?

https://github.com/honojs/hono/pull/3707/files#diff-e3ae0a2a0f7c0f9209a9627ba4da7f499f23e1873275b8d0c0f028e77d9cdee4R87-R93

[Soviut]

@yusukebe Yes, let's do that. I've updated the PR to warn if the target is form but content-type header is missing either multipart/form-data or application/x-www-url-encoded

[EdamAme-x]

I think that this may be an option.

Because a threat actor's malicious request could cause a warning to be logged to the console in a production environment.

[Soviut]

Because a threat actor's malicious request could cause a warning to be logged to the console in a production environment.

What is the concern with that? Wouldn't that help identify threat actors more easily if lots of warnings start showing up in logs?

[Soviut]

@EdamAme-x @yusukebe this seems to have stalled. Is there a security issue? if not, I'd love to see it merged.

[EdamAme-x]

What is the concern with that? Wouldn't that help identify threat actors more easily if lots of warnings start showing up in logs?

Apologies if this is misplaced.

For example, if someone with malicious intent deliberately sends a request with the Content-Type set incorrectly to the root where the validator is located, it might trigger a warning.

This could result in the logs being affected, which may lead to increased load.

While the administrator would likely notice the attack in the logs, there might not be a clear way to address the issue fully. Even if the IP is blocked, the potential risk of attack would still remain.

[EdamAme-x]

I think the solution to this is to allow option to decide whether to log or not.

The user can choose whether or not to generate warnings by environment variables, etc.

In the production environment, the log should not be emitted, but in the development environment, it should be emitted.

validator(
  "json",
  (...) => {...},
  {
    showWarning: process.env.NODE_ENV === "development"
  }
)

However, it does increase the user load somewhat.
Default values must also be considered.

[EdamAme-x]

@yusukebe What do you think?

[Soviut]

@EdamAme-x An option to enable the warnings could be added, but I think that's out of scope for this particular PR. This PR is to address the issue that the invalid body is ignored, making it hard to debug.

I think a follow-up ticket is in order since it would require some additional decisions, docs updates, etc.

[EdamAme-x]

Okay, and for now, let's consider the following PRs.

[Soviut]

@yusukebe What should I do to get this merged?

[yusukebe]

@Soviut @EdamAme-x

I'm sorry for not getting back to you sooner. I've considered it, and I believe we should not accept this PR. As @EdamAme-x mentioned, this change will allow the expected warning messages to be shown. This is a vulnerability, and while usability is important, avoiding vulnerabilities is more important.

[Soviut]

@yusukebe @EdamAme-x Okay, I'd like to adjust this PR to include the setting that was mentioned earlier. Instead of showWarning I'm thinking just warn would be cleaner. This would be set to true by default.

[EdamAme-x]

Assuming we accept this PR, it is doubtful that users will notice this option.
Perhaps we should consider setting the default to false.

[Soviut]

@EdamAme-x Defaulting to false ultimately ends up back where we started, with a difficult to debug scenario that the warning solves if we set it to true by default.

I'm willing to write documentation about this that should be discoverable by anybody searching for the warning and the associated options to turn it off.

We could also update the warning messages to include a hint about setting warn to false to mute the warnings.

[EdamAme-x]

I see, however, it may cause problems that are unnoticeable due to dependency updates, etc.
So it may be extreme, but there may be options such as introducing it in v5.

However, it may still be a difficult to introduce...

[Soviut]

@EdamAme-x I see. Is there any reason we couldn't return a 415 Unsupported Media Type error from this? Then the only person receiving it is the sender who ultimately is the one who needs to know their headers are set incorrectly.

We could ignore the validation function passed as the 2nd arg and return the 415, preventing the validation function from returning misleading errors of its own.

6.5.13. 415 Unsupported Media Type
The 415 (Unsupported Media Type) status code indicates that the
origin server is refusing to service the request because the payload
is in a format not supported by this method on the target resource.
The format problem might be due to the request's indicated
Content-Type or Content-Encoding, or as a result of inspecting the
data directly.

[EdamAme-x]

Interesting idea.
It might make more sense to return a response than to issue a warning.
Also, I don't think this is a disruptive change. (as long as the validator is used correctly).

@yusukebe What do you think?

[Soviut]

@EdamAme-x glad to hear. I agree, the warning was my 2nd choice. I originally wanted to return an error like this but I think I phrased it as an exception earlier. An error response is what I meant.

[Soviut]

@EdamAme-x I've updated the PR to replace the console warnings with HttpException(415, ...) and truncated error messages. This is in keeping with similar error messages in other parts of the validator.

[EdamAme-x]

@Soviut Can you also write tests on this?

[Soviut]

@EdamAme-x I've updated your suggestion. Let me know what you think.

Just note that there are prior error messages like

const message = 'Malformed JSON in request body'

and this one that gets dynamically built

let message = 'Malformed FormData request.'
message += e instanceof Error ? ` ${e.message}` : ` ${String(e)}`

[Soviut]

@Soviut Can you also write tests on this?

Yeah, once we think this looks good I can write some tests. Hopefully some already exist for these cases and just need to be updated.

EDIT: looks like there are tests that just need to be updated https://github.com/Soviut/hono/blob/patch-1/src/validator/validator.test.ts#L128

[EdamAme-x]

@EdamAme-x I've updated your suggestion. Let me know what you think.

I think the current one is fine.

As for the test, I think it could be changed.
I don't think it can be a destructive change if the validator is used correctly.

[Soviut]

@EdamAme-x I've updated the tests. The JSON ones only needed the status code updated. However, the FormData tests were (as far as I could tell) missing the same tests, so I replicated them.

[EdamAme-x]

LGTM

[Soviut]

LGTM

Great. We'll need to kick off the checks again.

@yusukebe let me know if anything else needs to be done.

EDIT: Running the tests locally I see that some are failing. I'll have those fixed tomorrow.

[EdamAme-x]

Good night, @yusukebe.
The PR direction has been changed to return a response with status code 415.
What do you think?

I think that this is a safer, more web standards-based approach.

[Soviut]

@EdamAme-x something I've realized is that this approach doesn't quite work when multiple validators are used. Because you can have the following...

  app.post(
    '/',
    validator('json', (value) => value),
    validator('form', (value) => value),
    async (c) => {
      const jsonData = c.req.valid('json')
      const formData = c.req.valid('form')
      return c.json({
        json: jsonData,
        form: formData,
      })
    }
  )

...each validator validates in isolation, meaning if you test for the json target, the form validator will fail.

This seems to be why the validators were initially failing silently; so they could pass-through. I'm going to see if I can come up with a way of handling this.

[EdamAme-x]

I wonder if there is a use case for a single endpoint to receive json/form.
(However, this is not the case if it is used as middleware)

This may be a bit of a difficult problem.

[EdamAme-x]

This would be a destructive change, but what about telling them to use this middleware if they are using different target validators on the same route?
https://hono.dev/docs/middleware/builtin/combine#some

[EdamAme-x]

@Soviut What do you think?

[Soviut]

I’ll have a look later this evening.

[Soviut]

@EdamAme-x I could definitely update the tests to work with some() where appropriate.

It is a breaking change so I suppose it might need to wait for Hono 5.0? If so, is there a roadmap? I searched the issues but the last roadmap was for 3.1.0. #968

[EdamAme-x]

There is currently no official roadmap.

Hi @usualoma, what do you think about the idea of using some of these use cases?
(It would be a breaking change, so it might be realistic to release it in Hono v5)

  app.post(
    '/',
    some(
      validator('json', (value) => value),
      validator('form', (value) => value),
    ),
    async (c) => {
      // ...
    }
  )

I am also wondering if Combine middleware can handle validator types well.

[Soviut]

@EdamAme-x combine middleware definitely could use some more docs because it affects how c.req.valid() works.

[EdamAme-x]

It seems that Combine needs to better support combining validator types, etc.