lua-Spore: Patch to accept more content-types as json#2262
Conversation
Refer to discussion 14710 and issue 14596
Frenzie
left a comment
There was a problem hiding this comment.
Has upstream been informed already? I regard a local patch as a necessary evil but not as the first choice.
| + if find(header, m['content-type'], 1, true) then | ||
| + return true | ||
| + end | ||
| + if string.match(header, "application/[%w%p]-json") then |
There was a problem hiding this comment.
This strikes me as not quite right. It should at least be "application/[%w%p]-%+json", though I'd possibly lean towards something even stricter like "application/[%w.]-%+json" (concretely, it shouldn't contain pretty much any of ()<>@,;:\\"/[]?={} iirc).
As an aside but I think that's already an issue regardless, presumably it should split on ; if present to ignore something like ; charset=UTF-8 if present without regarding application/jsonbutnotreallyhaha as JSON.
There was a problem hiding this comment.
You are right, I was allowing too much, I have updated the pull request with a more strict check + also handling the ';' part.
Upstream has been informed about a month ago in issue 27. I decided to have a go at fixing it myself because this sync issue has been around a long time. I'll try and do a pull request upstream as well, but I wanted to fix koreader first ;) |
| + -- return true | ||
| + -- end | ||
| + -- | ||
| + for part in string.gmatch(header, '[^;]+') do |
There was a problem hiding this comment.
This should be:
local mime_type = match(header, '^[^;]+')There was a problem hiding this comment.
I had put this in because I wanted to handle possibilities where the media-type was after a ;, but the spec does not allow this apparently.
There was a problem hiding this comment.
Yes, that's where the parameters go. I'm not sure otoh if / is allowed in a parameter, but it doesn't particularly matter since it shouldn't be considered for the job regardless. ^_^
content-type specicifies that it should start with the mime-type, no parameters can be present before it
| + if find(header, m['content-type'], 1, true) then | ||
| + return true | ||
| + end |
There was a problem hiding this comment.
This old code shouldn't remain here since that features the application/jsonbutnotreally issue.
If such a shortcut is desirable maybe it could be made into something like this:
if header == m['content-type'] or find(header, m['content-type'] .. ';', 1, true) then
return true
endThough I'm inclined to think it shouldn't be more complicated than this, and leave further handling to the rest of the code.
if header == m['content-type'] then
return true
end|
|
||
| +local has_json_mime_type = function(header) | ||
| + local mime_type = match(header, '[^;]+') | ||
| + return match(mime_type, "^%s*application/[%w.]-%+json%s*$") |
There was a problem hiding this comment.
Validity-wise neither of those whitespace matches make sense, do they? It also doesn't seem that the one at the start would belong in this part of the code.
It seems fairly harmless, but I don't know how upstream would feel about it.
…der-base#2262) Refer to discussion [14710](koreader#14710 (comment)) Fixes issue [14596](koreader#14596 (comment))
Refer to discussion 14710
Fixes issue 14596
This change is