Skip to content

Conversation

@akosyakov
Copy link
Member

@akosyakov akosyakov commented Feb 13, 2023

Description

Strict same site origin for /api/gitpod endpoint

How to test

  1. Trying to create a websocket connection from workspace origin should fail new WebSocket("wss://ak-strict-server.preview.gitpod-dev.com/api/gitpod")

Release Notes

NONE

Documentation

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • leeway-no-cache
    leeway-target=components:all
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-slow-database
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-ak-strict-server.1 because the annotations in the pull request description changed
(with .werft/ from main)

@iQQBot
Copy link
Contributor

iQQBot commented Feb 13, 2023

/werft run

👍 started the job as gitpod-build-ak-strict-server.2
(with .werft/ from main)

@jeanp413 jeanp413 marked this pull request as ready for review February 13, 2023 17:55
@jeanp413 jeanp413 requested a review from a team February 13, 2023 17:55
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 13, 2023
@jeanp413
Copy link
Member

/hold

@AlexTugarev AlexTugarev requested a review from geropl February 14, 2023 09:22
@AlexTugarev
Copy link
Member

@geropl, just FYI, I did a quick test and this kind of change does provide an effective blocking.

// Only exception: If no Origin header is set, skip the check!
export const isAllowedWebsocketDomain = (originHeader: string, gitpodHostName: string, strict: boolean): boolean => {
if (!originHeader) {
// TODO(gpl) Can we get rid of this dependency alltogether?
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how to read that comment. Could you clarify?

Is it planned to disallow creation of the WebSocket connection from clients other than browser agents?

If not, this check remains less meaningful outside of browser, i.e. from other clients it would be possible to set origin to any value. This is the reason to skip here and provide a signal, that this is browser-only security mechanism.

Copy link
Member

Choose a reason for hiding this comment

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

Is it planned to disallow creation of the WebSocket connection from clients other than browser agents?

Yes, at least for session/cookie authentiction. BearerAuth could keep working with the check !originHeader && !strict.

I basically want to have more insight into how this branch is used. 👍

@geropl
Copy link
Member

geropl commented Feb 14, 2023

@akosyakov @AlexTugarev Could somebody help with testing local IDEs? I get "SSH test connection error: ip:22 connection refused"

@akosyakov
Copy link
Member Author

@akosyakov @AlexTugarev Could somebody help with testing local IDEs? I get "SSH test connection error: ip:22 connection refused"

cc @iQQBot maybe you can help?

@geropl
Copy link
Member

geropl commented Feb 14, 2023

Done with testing, just worked after @iQQBot fixed the ingress proxy 🙏

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Thanks @akosyakov , ended up taking over most of your code with one change and some more comments/logs. 🙏

Tested and LGTM ✔️

@geropl
Copy link
Member

geropl commented Feb 14, 2023

/unhold

@roboquat roboquat merged commit 2c4390a into main Feb 14, 2023
@roboquat roboquat deleted the ak/strict_server branch February 14, 2023 14:00
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/L team: webapp Issue belongs to the WebApp team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants