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

fix: linting issues #738

Merged
merged 3 commits into from
Feb 13, 2025
Merged

fix: linting issues #738

merged 3 commits into from
Feb 13, 2025

Conversation

nickfloyd
Copy link
Contributor

This is a blocker for the release to get previous merged work shipped.

@nickfloyd nickfloyd requested review from gr2m and wolfy1339 February 13, 2025 17:58
Copy link

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

There were ReDos vulnerabilities in those packages
@wolfy1339 wolfy1339 enabled auto-merge (squash) February 13, 2025 18:05
@nickfloyd
Copy link
Contributor Author

Verified tests fail with previous versions of
"@octokit/endpoint": "^10.0.0",
"@octokit/request-error": "^6.0.1",

@wolfy1339
Copy link
Member

I am seeing this locally as well. response.url is an empty string in the code, but is defined when i run fetch with the same url from a Node repl...

> await fetch("https://www.githubstatus.com/api/v2/status.json")
Response {
  status: 200,
  statusText: 'OK',
  headers: Headers {
    'content-type': 'application/json; charset=utf-8',
    'content-length': '215',
    connection: 'keep-alive',
    date: 'Thu, 13 Feb 2025 18:09:12 GMT',
    'x-download-options': 'noopen',
    'x-permitted-cross-domain-policies': 'none',
    'referrer-policy': 'strict-origin-when-cross-origin',
    'x-statuspage-version': '7edc5703ef05afb2a5eda55b3be76176673e3049',
    'strict-transport-security': 'max-age=259200',
    'x-statuspage-skip-logging': 'true',
    'access-control-allow-origin': '*',
    'cache-control': 'max-age=3, public',
    'x-pollinator-metadata-service': 'status-page-web-pages',
    'x-runtime': '0.049426',
    'accept-ranges': 'bytes',
    'x-content-type-options': 'nosniff',
    'x-xss-protection': '1; mode=block',
    'atl-traceid': 'fa5d78f462c64bad8bf75d0a81c6ac78',
    'atl-request-id': 'fa5d78f4-62c6-4bad-8bf7-5d0a81c6ac78',
    'report-to': '{"endpoints": [{"url": "https://dz8aopenkvv6s.cloudfront.net"}], "group": "endpoint-1", "include_subdomains": true, "max_age": 600}',
    nel: '{"failure_fraction": 0.001, "include_subdomains": true, "max_age": 600, "report_to": "endpoint-1"}',
    server: 'AtlassianEdge',
    etag: 'W/"4f979914dfba35b846c49883ff0e1feb"',
    vary: 'Accept,Accept-Encoding',
    'x-cache': 'Hit from cloudfront',
    via: '1.1 fbdc01f132101cb05310363b09502a86.cloudfront.net (CloudFront)',
    'x-amz-cf-pop': 'YUL62-P1',
    'x-amz-cf-id': 'H9TP5_pCul3H06Q7cpU1U_ajK2G7DuCUKfl5h29ID2nYAqC_czanAA==',
    age: '2'
  },
  body: ReadableStream { locked: false, state: 'readable', supportsBYOB: true },
  bodyUsed: false,
  ok: true,
  redirected: false,
  type: 'basic',
  url: 'https://www.github
  status.com/api/v2/status.json'
}

@nickfloyd
Copy link
Contributor Author

nickfloyd commented Feb 13, 2025

I verified that the test is getting data back:

{
  "page": {
    "id": "kctbh9vrtdwd",
    "name": "GitHub",
    "time_zone": "Etc/UTC",
    "updated_at": "2025-02-13T15:05:54.233Z",
    "url": "https://www.githubstatus.com",
  },
  "status": {
    "description": "All Systems Operational",
    "indicator": "none",
  },
}

but response.url is always empty string. But the body has the host: expect(response.data.page.url).toEqual("https://www.githubstatus.com");

Feels like a redirect issue possibly, but the response.status value is 200 technically correct, if it was a redirect I would've expected a 3xx

@wolfy1339
Copy link
Member

The last known good commit (even with updated versions of @octokit/request-error and @octokit/endpoint), was a0e96b3

The changes in 34ff07e seem to have broken the tests somehow

@nickfloyd
Copy link
Contributor Author

nickfloyd commented Feb 13, 2025

Yeah it's something about the fetch-wrapper change that it does not like:
old (works):

  • responseHeaders.link.match(/<([^>]+)>;\s*rel="deprecation"/);
    new (breaks the test):
  • responseHeaders.link.match(/<([^<>]+)>; *rel="deprecation"/);

Looks like the \s match is the problem (or lack of in the new source)

@nickfloyd
Copy link
Contributor Author

I think I found it... It looks like the ReDoS tests are polluting the other request tests. I'm working on a fix now.

@wolfy1339 wolfy1339 merged commit 6bb29ba into main Feb 13, 2025
8 checks passed
@wolfy1339 wolfy1339 deleted the fix-lint branch February 13, 2025 18:55
Copy link

🎉 This PR is included in version 9.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants