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

Do not send a body for a GET request #323

Conversation

drairi
Copy link
Contributor

@drairi drairi commented Aug 3, 2022

CircleCI doesn't like it

  • ❌ Tests added (not needed)
  • ✅ All tests pass
  • ✅ Branch is up to date and mergeable
  • ❌ README and documentation updated (not needed)

Changes

Do not send data with GET requests. CircleCI has begun intermittently to fail, if a data is sent (even if that data is "{}")

Verify

CircleCi::Project.new(org, project).recent_builds

CircleCI has started to return 403 Forbidden if this is sent with data.

You can also see the same effect in these curl requests. The first may fail, the second will not.

curl -X GET --data-binary "{}" "https://circleci.com/api/v1.1/project/github/org/project?circle-token=TOKEN"
curl -X GET "https://circleci.com/api/v1.1/project/github/org/project?circle-token=TOKEN"

CircleCI doesn't like it
@lukeredpath
Copy link

I've just spent an hour debugging this problem myself - our CI builds started failing on our Danger step where we are using this gem to fetch artefact information from Circle. Seems that Circle has changed their Cloudfront configuration to reject GET requests with a non-empty body.

@drairi
Copy link
Contributor Author

drairi commented Aug 3, 2022

It seems a bit intermittent, sometimes it works, sometimes it doesn't. I've asked them on twitter if they've changed anything, but no response as yet.

https://twitter.com/sermoa/status/1554817410099806208

My quick-hack solution is to monkeypatch the gem method like this:

module CircleCi
  class Request
    def connection(verb, body = {})
      req = Net::HTTP.const_get(verb.to_s.capitalize, false).new(@uri, DEFAULT_HEADERS)
      req.body = JSON.dump(body) unless verb == :get
      req
    end
  end
end

@drairi
Copy link
Contributor Author

drairi commented Aug 3, 2022

CircleCI made a change last week to reject jobs with a particular config. Their argument being, "It's never worked so we want to make it obvious that it's not doing what people think it might".

I guess it's a similar thing here. It makes no sense to send a body with a GET request .. but rather than silently ignoring it, they seem to want to fail in an obvious way.

I wish they were a bit more communicative about these breaking changes though, for those of us who have relied on things that were not quite right but still worked.

@zackse
Copy link

zackse commented Aug 3, 2022

We are observing the same behavior and can reproduce the issue with curl by supplying a Content-Length header on a GET request:

curl --http1.1 -H "Content-Length: 2" -s \
    https://circleci.com/api/v1.1/project/github/USERNAME/PROJECT/BUILD_NUM?circle-token=TOKEN

Thanks for posting a fix, @drairi!

@devinburnette
Copy link

having this problem as well, hope the fix is merged soon!

@drairi
Copy link
Contributor Author

drairi commented Aug 3, 2022

@mtchavez is this project still active?

@mtchavez mtchavez self-assigned this Aug 3, 2022
@mtchavez mtchavez added the bug label Aug 3, 2022
@mtchavez mtchavez merged commit 9325624 into mtchavez:master Aug 3, 2022
@devinburnette
Copy link

thanks @mtchavez for getting this merged in. I see there hasn't been a release in almost 3 years so I imagine the changeset is pretty large, but when do you think you'll have time to cut a new release with the latest fix?

@mtchavez
Copy link
Owner

mtchavez commented Aug 3, 2022

Version 2.1.0 of the gem is released with the fix

@drairi drairi deleted the fix-circleci-failing-on-get-requests-with-data branch August 4, 2022 07:24
@drairi
Copy link
Contributor Author

drairi commented Aug 4, 2022

Thank you so much for merging and releasing a new version! Tested and it works beautifully!

In case you're interested, we use thes gem in combination with Smashing Dashboard to show our CircleCI build and test results. All the screens round the office were looking very sad yesterday, and now all the colour is back and everyone is happy again! 😁

smashboard1

smashboard2

@lukeredpath
Copy link

Yes, thank you for the quick turnaround on this.

FYI, I had a response to my customer support ticket from CircleCI confirming they did make this change but having identified a "customer impact" they are temporarily reverting the change. It will be rolled out again with an announcement in advance.

@TheMetalCode
Copy link

Appreciate the quick turnaround on this!

kennyadsl added a commit to solidusio/solidus.io that referenced this pull request Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants