-
Notifications
You must be signed in to change notification settings - Fork 790
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
Content-Disposition utf8 encoding fix #4604
Content-Disposition utf8 encoding fix #4604
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow review. I had to reread the spec. And what an awful spec this one is!
final case class `Content-Disposition`(dispositionType: String, parameters: Map[String, String]) | ||
final case class `Content-Disposition`( | ||
dispositionType: String, | ||
parameters: Map[String, PtcEncodedString]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing is preventing us from creating AsciiString("🔥")
, so I'm not sure PctEncoding
belongs in the public API. All regular parameters should support an ASCII encoding, and all *
parameters are supposed to use the charset encoding. I think we should just store everything as decoded String
s.
Unfortunately, this simple model does not prevent us from creating invalid key names, or invalid values for keys that aren't the extended tokens. I don't really see a way to enforce that that results in a pleasant type to use.
The spec does say filename
and filename*
are case insensitive, and that duplicate keys aren't supported. It is silent on whether extension fields are case insensitive, but it's a good bet the answer is yes, so a Map[CIString, String]
seems appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very irritatingly, in RFC7578, we find the filename*
MUST NOT be used. So what's valid depends on whether this is a response header or a multipart part header. I don't think we can achieve full compliance with all these specs at once without turning this into http4Idris.
@@ -79,13 +83,36 @@ object `Content-Disposition` { | |||
new Renderable { | |||
def render(writer: Writer): writer.type = { | |||
writer.append(v.dispositionType) | |||
v.parameters.foreach(p => writer << "; " << p._1 << "=\"" << p._2 << '"') | |||
v.parameters.foreach(p => writer << "; " << p._1 << "=" << p._2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of nuance in rendering this correctly. I think the akka-http implementation is worth a look. Note how it renders the extended filename*
after filename
, which is important. It also synthesizes a filename*
where filename
is unsafe, which seems like a nice idea.
Thanks for input. hmm it looked easier at first, but I guess I have to do some RFC readings as well then. |
Ok prepared some code based on akka-http and went back to parameters defined just as Map[String,String]. I had some doubts to introduce keys as CIString as I thought it should be consistent with other headers that are using a Map. But after quick scan through rest of these i didnt find any so I guess we could proced with this change to CIString. Just let me know if the rest of changes are all right. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. Some of these headers are shockingly complicated!
|
||
object `Content-Disposition` { | ||
val safeChars = CharPredicate.Printable -- "%\"\\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably keep this private.
val renderExtFilename = | ||
v.parameters.get("filename").exists(!safeChars.matchesAll(_)) | ||
val withExtParams = | ||
if (renderExtFilename && !v.parameters.contains("filename*")) | ||
v.parameters + ("filename*" -> v.parameters("filename")) | ||
else v.parameters | ||
val withExtParamsSorted = | ||
if (withExtParams.contains("filename") && withExtParams.contains("filename*")) | ||
TreeMap[String, String]() ++ withExtParams | ||
else withExtParams | ||
|
||
writer.append(v.dispositionType) | ||
v.parameters.foreach(p => writer << "; " << p._1 << "=\"" << p._2 << '"') | ||
withExtParamsSorted.foreach { | ||
case (k, v) if k == "filename" => | ||
writer << "; " << k << '=' << '"' | ||
writer.eligibleOnly(v, keep = safeChars, placeholder = '?') << '"' | ||
case (k, v) if k.endsWith("*") => | ||
writer << "; " << k << '=' << "UTF-8''" << Uri.encode(v) | ||
case (k, v) => writer << "; " << k << "=\"" << v << '"' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar enough to the akka-http implementation that it's probably worth an attribution. We already mention them in NOTICES
since we've copied elsewhere from them.
OK, so my understanding is I should:
What about this change to convert properties key to CIString? I could really use this ci interpolator here introduced lately, otherwise it could be quite annoying to use. |
Oh, right. Yes, the Correct on the other two points. |
fixes #4513