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(server): clean up vestigial code from proxy #3640

Merged
merged 2 commits into from
Jun 1, 2021
Merged

fix(server): clean up vestigial code from proxy #3640

merged 2 commits into from
Jun 1, 2021

Conversation

jginsburgn
Copy link
Member

@jginsburgn jginsburgn commented Jan 28, 2021

When using the proxy feature of Karma, the target value can include the scheme. It was used to determine the https variable to be sent to the http-proxy .createProxyServer method. However, it is now disregarded by that package. Therefore, this PR cleans it up.

@karmarunnerbot
Copy link
Member

Build karma 507 completed (commit 3701f5f956 by @jginsburgn)

@AppVeyorBot
Copy link

Build karma 2904 completed (commit 3701f5f956 by @jginsburgn)

@jginsburgn jginsburgn changed the title fix(server/proxy): use protocol to determine agent fix(server/proxy): allow config.protocol to control the connection scheme. Jan 28, 2021
@karmarunnerbot
Copy link
Member

Build karma 506 completed (commit 3701f5f956 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 508 completed (commit 796b6a4738 by @jginsburgn)

@AppVeyorBot
Copy link

Build karma 2905 completed (commit 796b6a4738 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 507 completed (commit 796b6a4738 by @jginsburgn)

lib/middleware/proxy.js Outdated Show resolved Hide resolved
@devoto13
Copy link
Collaborator

devoto13 commented Jan 29, 2021

The fix looks correct to me. An example would be a path-only proxy when Karma is configured to run over https. E.g.

{
  protocol: 'https',
  proxies: {
    '/img/': '/base/test/images/'
  }
  ...
}
> url.parse('/base/test/images/')
Url {
  protocol: null,
  slashes: null,
  auth: null,
  host: null,
  port: null,
  hostname: null,
  hash: null,
  search: null,
  query: null,
  pathname: '/base/test/images/',
  path: '/base/test/images/',
  href: '/base/test/images/'
}

See Loading assets section (near the end): http://karma-runner.github.io/6.0/config/files.html

Can you add a test case based on the above example, please?

@johnjbarton johnjbarton requested review from devoto13 and removed request for johnjbarton February 1, 2021 17:51
@johnjbarton
Copy link
Contributor

Yes the test would clarify the use case

@jginsburgn jginsburgn changed the title fix(server/proxy): allow config.protocol to control the connection scheme. fix(server/proxy): allow config.protocol to control the connection scheme Feb 1, 2021
@karmarunnerbot
Copy link
Member

Build karma 584 completed (commit 8f56bf971a by @jginsburgn)

@AppVeyorBot
Copy link

Build karma 2981 completed (commit 8f56bf971a by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 583 completed (commit 8f56bf971a by @jginsburgn)

@jginsburgn
Copy link
Member Author

Upon trying to add a test for the use case that @devoto13 mentioned, I figured that it makes no sense to have a comms protocol (e.g. https or http) when proxying requests between paths. That is because the target path is in the local filesystem and no connection should be done.

fix(server/proxy): config.protocol can control connection scheme

fix(server/proxy): clean up unused logic
@AppVeyorBot
Copy link

Build karma 2982 completed (commit e17e5fbd22 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 585 completed (commit e17e5fbd22 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 584 completed (commit e17e5fbd22 by @jginsburgn)

@jginsburgn jginsburgn changed the title fix(server/proxy): allow config.protocol to control the connection scheme fix(server/proxy): clean up unused logic Apr 27, 2021
@AppVeyorBot
Copy link

Build karma 2983 completed (commit b16a33b503 by @jginsburgn)

@jginsburgn
Copy link
Member Author

Investigating more, the NPM module http-proxy that we are using, does not make use of the https variable that we were sending it. Also, it disregards the agent configuration. Therefore, I cleaned this logic up and its corresponding tests.

@karmarunnerbot
Copy link
Member

Build karma 586 completed (commit b16a33b503 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 585 completed (commit b16a33b503 by @jginsburgn)

@devoto13
Copy link
Collaborator

The naming is confusing.

I haven't really debugged it, but from a quick code scan the agent option is copied into requestOptions here and passed through here and here to here and eventually into agent option of the request() method from either http or https module, which you referenced in you comment.

@jginsburgn jginsburgn changed the title fix(server/proxy): clean up unused logic fix(server): clean up vestigial code from proxy May 4, 2021
@jginsburgn jginsburgn changed the title fix(server): clean up vestigial code from proxy fix(server): clean up vestigial code from proxy May 4, 2021
@jginsburgn jginsburgn assigned jginsburgn and unassigned gjyalpha May 4, 2021
@jginsburgn
Copy link
Member Author

jginsburgn commented May 4, 2021

@devoto13 you're right! I missed that path :/. Let me fix this PR.

When using the proxy feature of Karma, the target value can include the [scheme](https://tools.ietf.org/html/std66#section-3.1). It was used to determine the `https` variable to be sent to the [`http-proxy`](https://www.npmjs.com/package/http-proxy) `.createProxyServer` method. However, it is now disregarded by that package. Therefore, this PR cleans it up.
@AppVeyorBot
Copy link

Build karma 2984 completed (commit 87a026fe86 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 587 completed (commit 87a026fe86 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 586 completed (commit 87a026fe86 by @jginsburgn)

@jginsburgn jginsburgn assigned devoto13 and gjyalpha and unassigned jginsburgn May 5, 2021
Copy link
Collaborator

@devoto13 devoto13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jginsburgn jginsburgn removed the request for review from johnjbarton May 5, 2021 18:45
@AppVeyorBot
Copy link

Build karma 2985 completed (commit 7fa2499251 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 588 completed (commit 7fa2499251 by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 587 completed (commit 7fa2499251 by @jginsburgn)

@AppVeyorBot
Copy link

Build karma 2986 completed (commit 87beaa51db by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 589 completed (commit 87beaa51db by @jginsburgn)

@karmarunnerbot
Copy link
Member

Build karma 588 completed (commit 87beaa51db by @jginsburgn)

@jginsburgn jginsburgn merged commit f4aeac3 into karma-runner:master Jun 1, 2021
karmarunnerbot pushed a commit that referenced this pull request Jun 1, 2021
## [6.3.3](v6.3.2...v6.3.3) (2021-06-01)

### Bug Fixes

* **server:** clean up vestigial code from proxy ([#3640](#3640)) ([f4aeac3](f4aeac3)), closes [/tools.ietf.org/html/std66#section-3](https://github.com//tools.ietf.org/html/std66/issues/section-3)
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 17, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [karma](https://karma-runner.github.io/) ([source](https://github.com/karma-runner/karma)) | devDependencies | minor | [`6.3.20` -> `6.4.0`](https://renovatebot.com/diffs/npm/karma/6.3.20/6.4.0) |

---

### Release Notes

<details>
<summary>karma-runner/karma</summary>

### [`v6.4.0`](https://github.com/karma-runner/karma/blob/HEAD/CHANGELOG.md#&#8203;640-httpsgithubcomkarma-runnerkarmacomparev6320v640-2022-06-14)

[Compare Source](karma-runner/karma@v6.3.20...v6.4.0)

##### Features

-   support SRI verification of link tags ([dc51a2e](karma-runner/karma@dc51a2e))
-   support SRI verification of script tags ([6a54b1c](karma-runner/karma@6a54b1c))

#### [6.3.20](karma-runner/karma@v6.3.19...v6.3.20) (2022-05-13)

##### Bug Fixes

-   prefer IPv4 addresses when resolving domains ([e17698f](karma-runner/karma@e17698f)), closes [#&#8203;3730](karma-runner/karma#3730)

#### [6.3.19](karma-runner/karma@v6.3.18...v6.3.19) (2022-04-19)

##### Bug Fixes

-   **client:** error out when opening a new tab fails ([099b85e](karma-runner/karma@099b85e))

#### [6.3.18](karma-runner/karma@v6.3.17...v6.3.18) (2022-04-13)

##### Bug Fixes

-   **deps:** upgrade socket.io to v4.4.1 ([52a30bb](karma-runner/karma@52a30bb))

#### [6.3.17](karma-runner/karma@v6.3.16...v6.3.17) (2022-02-28)

##### Bug Fixes

-   **deps:** update colors to maintained version ([#&#8203;3763](karma-runner/karma#3763)) ([fca1884](karma-runner/karma@fca1884))

#### [6.3.16](karma-runner/karma@v6.3.15...v6.3.16) (2022-02-10)

##### Bug Fixes

-   **security:** mitigate the "Open Redirect Vulnerability" ([ff7edbb](karma-runner/karma@ff7edbb))

#### [6.3.15](karma-runner/karma@v6.3.14...v6.3.15) (2022-02-05)

##### Bug Fixes

-   **helper:** make mkdirIfNotExists helper resilient to concurrent calls ([d9dade2](karma-runner/karma@d9dade2)), closes [/github.com/karma-runner/karma-coverage/issues/434#issuecomment-1017939333](https://github.com//github.com/karma-runner/karma-coverage/issues/434/issues/issuecomment-1017939333)

#### [6.3.14](karma-runner/karma@v6.3.13...v6.3.14) (2022-02-05)

##### Bug Fixes

-   remove string template from client code ([91d5acd](karma-runner/karma@91d5acd))
-   warn when `singleRun` and `autoWatch` are `false` ([69cfc76](karma-runner/karma@69cfc76))
-   **security:** remove XSS vulnerability in `returnUrl` query param ([839578c](karma-runner/karma@839578c))

#### [6.3.13](karma-runner/karma@v6.3.12...v6.3.13) (2022-01-31)

##### Bug Fixes

-   **deps:** bump log4js to resolve security issue ([5bf2df3](karma-runner/karma@5bf2df3)), closes [#&#8203;3751](karma-runner/karma#3751)

#### [6.3.12](karma-runner/karma@v6.3.11...v6.3.12) (2022-01-24)

##### Bug Fixes

-   remove depreciation warning from log4js ([41bed33](karma-runner/karma@41bed33))

#### [6.3.11](karma-runner/karma@v6.3.10...v6.3.11) (2022-01-13)

##### Bug Fixes

-   **deps:** pin colors package to 1.4.0 due to security vulnerability ([a5219c5](karma-runner/karma@a5219c5))

#### [6.3.10](karma-runner/karma@v6.3.9...v6.3.10) (2022-01-08)

##### Bug Fixes

-   **logger:** create parent folders if they are missing ([0d24bd9](karma-runner/karma@0d24bd9)), closes [#&#8203;3734](karma-runner/karma#3734)

#### [6.3.9](karma-runner/karma@v6.3.8...v6.3.9) (2021-11-16)

##### Bug Fixes

-   restartOnFileChange option not restarting the test run ([92ffe60](karma-runner/karma@92ffe60)), closes [#&#8203;27](karma-runner/karma#27) [#&#8203;3724](karma-runner/karma#3724)

#### [6.3.8](karma-runner/karma@v6.3.7...v6.3.8) (2021-11-07)

##### Bug Fixes

-   **reporter:** warning if stack trace contains generated code invocation ([4f23b14](karma-runner/karma@4f23b14))

#### [6.3.7](karma-runner/karma@v6.3.6...v6.3.7) (2021-11-01)

##### Bug Fixes

-   **middleware:** replace %X_UA_COMPATIBLE% marker anywhere in the file ([f1aeaec](karma-runner/karma@f1aeaec)), closes [#&#8203;3711](karma-runner/karma#3711)

#### [6.3.6](karma-runner/karma@v6.3.5...v6.3.6) (2021-10-25)

##### Bug Fixes

-   bump vulnerable ua-parser-js version ([6f2b2ec](karma-runner/karma@6f2b2ec)), closes [#&#8203;3713](karma-runner/karma#3713)

#### [6.3.5](karma-runner/karma@v6.3.4...v6.3.5) (2021-10-20)

##### Bug Fixes

-   **client:** prevent socket.io from hanging due to mocked clocks ([#&#8203;3695](karma-runner/karma#3695)) ([105da90](karma-runner/karma@105da90))

#### [6.3.4](karma-runner/karma@v6.3.3...v6.3.4) (2021-06-14)

##### Bug Fixes

-   bump production dependencies within SemVer ranges ([#&#8203;3682](karma-runner/karma#3682)) ([36467a8](karma-runner/karma@36467a8)), closes [#&#8203;3680](karma-runner/karma#3680)

#### [6.3.3](karma-runner/karma@v6.3.2...v6.3.3) (2021-06-01)

##### Bug Fixes

-   **server:** clean up vestigial code from proxy ([#&#8203;3640](karma-runner/karma#3640)) ([f4aeac3](karma-runner/karma@f4aeac3)), closes [/tools.ietf.org/html/std66#section-3](https://github.com//tools.ietf.org/html/std66/issues/section-3)

#### [6.3.2](karma-runner/karma@v6.3.1...v6.3.2) (2021-03-29)

##### Bug Fixes

-   fix running tests in IE9 ([#&#8203;3668](karma-runner/karma#3668)) ([0055bc5](karma-runner/karma@0055bc5)), closes [/github.com/karma-runner/karma/blob/026fff870913fb6cd2858dd962935dc74c92b725/client/main.js#L14](https://github.com//github.com/karma-runner/karma/blob/026fff870913fb6cd2858dd962935dc74c92b725/client/main.js/issues/L14) [#&#8203;3665](karma-runner/karma#3665)

#### [6.3.1](karma-runner/karma@v6.3.0...v6.3.1) (2021-03-24)

##### Bug Fixes

-   **client:** clearContext after complete sent ([#&#8203;3657](karma-runner/karma#3657)) ([c0962e3](karma-runner/karma@c0962e3))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1412
Reviewed-by: Epsilon_02 <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
When using the proxy feature of Karma, the target value can include the [scheme](https://tools.ietf.org/html/std66#section-3.1). It was used to determine the `https` variable to be sent to the [`http-proxy`](https://www.npmjs.com/package/http-proxy) `.createProxyServer` method. However, it is now disregarded by that package. This change cleans it up.
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
## [6.3.3](karma-runner/karma@v6.3.2...v6.3.3) (2021-06-01)

### Bug Fixes

* **server:** clean up vestigial code from proxy ([karma-runner#3640](karma-runner#3640)) ([f4aeac3](karma-runner@f4aeac3)), closes [/tools.ietf.org/html/std66#section-3](https://github.com//tools.ietf.org/html/std66/issues/section-3)
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