Access tokens should not be sent in multiple fields

[jamietanna]

Describe the bug

Access tokens are being sent in both the Authorization header, and in the form body. This is not correct, and some OAuth2-compliant resource servers will reject requests.

Sample, captured, request, with sanitised access tokens:

{
  "url" : "/micropub",
  "method" : "POST",
  "clientIp" : "82.0.19.204",
  "headers" : {
    "Authorization" : "Bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.",
    "Accept" : "application/json",
    "User-Agent" : "Dalvik/2.1.0 (Linux; U; Android 10; Pixel 3a Build/QP1A.191005.007)",
    "X-Forwarded-For" : "82.0.19.204",
    "Host" : "localhost:8080",
    "Accept-Encoding" : "gzip",
    "Content-Length" : "872",
    "Content-Type" : "multipart/form-data;boundary=apiclient-1571938860167"
  },
  "cookies" : { },
  "browserProxyRequest" : false,
  "loggedDate" : 1571938861507,
  "body" : "--apiclient-1571938860167\r\nContent-Disposition: form-data; name=\"access_token\"\r\n\r\neyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.\r\n--apiclient-1571938860167\r\nContent-Disposition: form-data; name=\"h\"\r\n\r\nentry\r\n--apiclient-1571938860167\r\nContent-Disposition: form-data; name=\"published\"\r\n\r\n2019-10-24T18:41:00+0100\r\n--apiclient-1571938860167\r\nContent-Disposition: form-data; name=\"post-status\"\r\n\r\npublished\r\n--apiclient-1571938860167\r\nContent-Disposition: form-data; name=\"content\"\r\n\r\nWibble\r\n--apiclient-1571938860167--\r\n",
  "scheme" : "http",
  "host" : "localhost",
  "port" : 8080,
  "loggedDateString" : "2019-10-24T17:41:01Z",
  "queryParams" : { }
}

To Reproduce

Set up a stub server in place of a micropub server, and observe the HTTP request received.

Expected behavior

Access tokens should be sent in one place.

Screenshots

N/A

Smartphone (please complete the following information):

Google Pixel 3A

Additional context

This depends on the resource server and the error they return, which is why some servers will not see this issue.

[swentel]

Yeah, I think I did this because the micropub endpoint from Wordpress required that. Not sure if I could remove it. @dshanske can you confirm ?

I think the other framework implementations (drupal for sure) are fine with header, so I'd be fine to only use that.

[swentel]

Another option of course is to make it configurable, defaulting to sending it in the header.

[jamietanna]

Ah yes I've just been looking through the code and saw the comment about WordPress. I think making it configurable would be best, letting folks decide how it's sent.

It may be worth warning users on their first run of the upgraded app that they're using the new defaults, and to allow them to set it there and then, otherwise WordPress users would suddenly have issues publishing content?

[dshanske]

We don't require it. We support either. But some servers block the header. Suggest it default to header only...

[jamietanna]

Default to header sounds good!

[swentel]

fixed in dev

[jamietanna]

Awesome, thanks very much for being so responsive! Do I remember correctly that the release is automatic at the end of the day?

[swentel]

Oh no, that's on a randomly selected moment I choose, so not really a fixed release schedule :)

That said, I'll do a release next week. After that I'm going to test a big new feature myself for a few weeks, so I'll make sure this one gets out first.

[jamietanna]

Ah OK cool. Any idea when next week? Or if there's some way to get it running as a beta test?

I'm unable to use the app with my own server unfortunately until I've got this, but I realise it's asking a lot for an out of band release

[swentel]

oh, in that case, pushed a new release :)

[jamietanna]

Amazing, thank you!