Drop special headers from redirected requests

AliSoftware / OHHTTPStubs

Stub your network requests easily! Test your apps with fake network data and custom response time, response code and headers!

MIT License

5.03k stars 601 forks source link

Drop special headers from redirected requests #289

Closed sberrevoets closed 5 years ago

sberrevoets commented 5 years ago

Checklist

[x] I've checked that all new and existing tests pass
[x] I've updated the documentation if necessary
[x] I've added an entry in the CHANGELOG to credit myself

Description

The change in https://github.com/AliSoftware/OHHTTPStubs/pull/263 means a redirected request had the same headers as the original request (because it was a copy and not a brand new NSURLRequest instance).

While the new behavior was correct, it didn't account for "special headers" that URLSession "handles". The docs aren't entirely clear what that means, but we've at least confirmed it removes the "Authorization" headers as that poses a security risk, so this PR removes all those headers + a test.

keith commented 5 years ago

I think these test failures are related to your changes @sberrevoets ? It looks like master is mostly green (besides some flakes)

sberrevoets commented 5 years ago

I was able to reproduce this once (via rake, I never saw them fail in Xcode 9.4.1), but now even rake is succeeding locally.

@AliSoftware any idea what might be going on?

AliSoftware commented 5 years ago

I think the build failures might be related to the race condition bug we discovered in URLSession in https://github.com/AliSoftware/OHHTTPStubs/issues/230#issuecomment-380931933 (don't hesitate to duplicate the radar to help us make Apple fix it btw)

sberrevoets commented 5 years ago

Ah thanks for the heads up, I've duped the radar. What would you like to see here in the meantime?

sberrevoets commented 5 years ago

Friendly ping @AliSoftware :)

AliSoftware commented 5 years ago

Sorry for the late reply. I honestly don't really know. I mean those redirect tests are probably always gonna fail until that Apple bug gets fixed or we find a workaround. So maybe we should merge that anyway given that the failures are not our fault?

Or a better solution might be to introduce a small delay in the redirection (like what we done in the other PR to confirm the bug was a race condition) as it seemed to at least reduce a lot the bug from happening; even if that means introducing a delay and slowing down stuff a little, that's probably better than failing 70% of the time…?

sberrevoets commented 5 years ago

Yeah that's a very good question. For me it'd probably depend on how much it fails. If it really is 70% of the time right now then adding a delay seems worth it. If it's every now and then, it might be possible to (somehow) auto-retry the affected tests, or take a simpler approach of adding a comment explaining the problem.

I'm also open to alternatives, framework bugs like these are a huge PITA 😕

jeffctown commented 5 years ago

@sberrevoets - we're reviewing all outstanding PRs before making a release, and then updating the version of Swift being used. the hope is to do all of this before Xcode 10.2 comes out, so our timeline is a bit tight.

If you are still interested in getting this in, please resolve the conflicts. If you are not interested, having that feedback would be helpful too.

sberrevoets commented 5 years ago

Happy to resolve conflicts (though can't promise I'll have time for that before the 10.2 release), but how depends on what approach is preferable. Based on the conversation above I'm not sure if there's a clear path forward.

jeffctown commented 5 years ago

@sberrevoets - if you mean the failing tests - those were disabled in CI (#301 & #302), so you don't need to worry about that part. We only need the conflicts resolved. If you want to give me permission to commit to your fork's master branch I don't mind resolving them myself. the tests should pass once the conflicts are resolved.

sberrevoets commented 5 years ago

Ah I missed that, that makes resolving conflicts a bit easier. Pushed an update but also gave edit access just in case.

jeffctown commented 5 years ago

@AliSoftware I updated this branch with a for loop, and I still see the conditional tests.

AliSoftware commented 5 years ago

I was talking about a new test case that @sberrevoets wrote in his initial commits, and that I remember about from when I reviewed the diff on that PR before the conflict resolution today. That new test case which was added in that PR was essentially creating a request, with all the headers of interest here + another header — named "preserved" iirc —, and after using that dummy request and redirecting it, did XCTAssertNil() on all proper headers to check they were properly removed and an XCTAssertEqual() on the one named "preserved" to check it wasn't removed.

That test case seems not to be there anymore (or is my GitHub view of the PR diff on my phone making me missing something?). It probably was removed by the force-push done by @sberrevoets today I guess. While I think it would be worth to have that test case back (even if it ends up not being run on CI because we disabled them there, but they would still be able to be run locally in Xcode)

jeffctown commented 5 years ago

Oh! My bad man. I thought you meant the buggy test.

_{Sent with GitHawk}

sberrevoets commented 5 years ago

I did remove that test case because it was one of the conflicts. Working on adding that back now.

sberrevoets commented 5 years ago

Added that back and ran it locally - it still passed.

AliSoftware commented 5 years ago

Thanks @sberrevoets :tada:

sberrevoets commented 5 years ago

Thanks for the reviews and suggestions!