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 option to decide what to do on invalid UTF-8 urlencoded params #1200

Closed
wants to merge 24 commits into from

Conversation

bradhanks
Copy link
Contributor

added error_code and byte_position to validate_utf8!
added ascii optimization as found in String.valid?
wrote tests for changes
edited existing tests to incorporate "in position #{byte_position}
added documentation

error: @utf8_error_code,
context: context,
byte: byte
)
Copy link
Member

Choose a reason for hiding this comment

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

It should always raise, and not behave differently based on the format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What information should be in the exception? I guess I'm not exactly clear on what exactly should change with different error codes.

Copy link
Member

Choose a reason for hiding this comment

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

If you want a way to not raise, then we should create a validate_utf8 that returns {:error, Exception} without raising. But we should not use the status code to control that. It is unclear what is the ultimate gain here.

@josevalim
Copy link
Member

There are many changes here. Can you please keep add them granularly? Although I don't think there is a need for position or to change the return type based on the code. It is a function with !, it should always raise.

@bradhanks
Copy link
Contributor Author

bradhanks commented Dec 27, 2023

Something like this?


  defp do_validate_utf8!(<<byte, _::bits>>, exception, context, error_code, byte_position) do
    case error_code do
      500 ->
        raise exception,
              "Internal Server Error (500): invalid UTF-8 on #{context}, got byte #{byte}"

      404 ->
        raise exception,
              "Page Not Found (404): invalid UTF-8 on #{context}, got byte #{byte}"

      error_code when error_code in 100..999 ->
        raise exception,
              "Status code #{error_code}: invalid UTF-8 on #{context}, got byte #{byte}"
    end
  end

@josevalim
Copy link
Member

No, that does not change how the server is going to respond in any way. Can you take a step back and remind us what is your end goal? :) If you want to change the exception, it would be something like:

defexception [plug_status: Application.compile_env(...)]

When defining the exception being raised.

@@ -1,4 +1,6 @@
defmodule Plug.Conn.Utils do
require Logger

Choose a reason for hiding this comment

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

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.

3 participants