-
Notifications
You must be signed in to change notification settings - Fork 1
feat: allow customizing workspace directory #2
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
Conversation
Allow specifying where code-server open
@@ -17,12 +17,18 @@ do | |||
code-server --install-extension "$extension" | |||
done | |||
|
|||
CODE_SERVER_WORKSPACE="$_REMOTE_USER_HOME" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$_REMOTE_USER_HOME
should always be set
src/code-server/install.sh
Outdated
cat > /usr/local/bin/code-server-entrypoint \ | ||
<< EOF | ||
#!/usr/bin/env bash | ||
set -e | ||
|
||
su $_REMOTE_USER -c 'code-server --bind-addr "$HOST:$PORT" \$ARGS' | ||
su $_REMOTE_USER -c 'code-server --bind-addr "$HOST:$PORT" $ARGS "$CODE_SERVER_WORKSPACE"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving from \$ARGS
to $ARGS
because escaping here meant that the script written to file would have the literal $ARGS
in it instead of the expanded result, which was not intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does ARGS come from and what does it contain? I wonder if we need to worry about quoting and/or newlines?
Right now a '
, "
or \n
would/could break the su command, example:
root@5110bc305062:/# su root -c 'echo hi
>
> there'
hi
bash: -c: line 2: syntax error near unexpected token `newline'
bash: -c: line 2: `>'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARGS
comes from the feature. A user could supply additional arguments here that are not covered by the feature. It could contain anything but should be in the form of additional command line arguments.
Maybe we need to escape this? (or just drop it altogether)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, maybe we could use eval for simplicity?
> ARGS="--foo \"some thing\" --'bar'"
> eval "declare -a args=($ARGS)"
> declare -p args
declare -a args=([0]="--foo" [1]="some thing" [2]="--bar")
Used like this:
code-server "${args[@]}"
You can still do something sneaky, like:
ARGS="$(rm -rf /)"
But I don't know if we need care? Someone could just as well include a malicious feature? 🤷🏻♂️
The alternative is that we encode supported code-server flags/opts as explicit feature options, which lets us drop ARGS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL of eval
😄
I think the best course of action would be to ensure we support every CLI flag with a feature and drop ARGS
entrypoint=$(cat /usr/local/bin/code-server-entrypoint) | ||
check "code-server workspace" grep $'\'code-server.*"/home"\'' <<<"$entrypoint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of a better way to test this other than to grep the created entry point to ensure the directory is in the command to launch code-server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to verify more of the cmdline? Ports, args, etc?
Alt suggestion (to useless use of cat 😼):
entrypoint=$(cat /usr/local/bin/code-server-entrypoint) | |
check "code-server workspace" grep $'\'code-server.*"/home"\'' <<<"$entrypoint" | |
check "code-server workspace" grep $'\'code-server.*"/home"\'' < /usr/local/bin/code-server-entrypoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each separate feature test could do their own verifications of this yeah (my next PR is using this approach).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned about edge cases with the entrypoint, but otherwise LGTM.
src/code-server/install.sh
Outdated
cat > /usr/local/bin/code-server-entrypoint \ | ||
<< EOF | ||
#!/usr/bin/env bash | ||
set -e | ||
|
||
su $_REMOTE_USER -c 'code-server --bind-addr "$HOST:$PORT" \$ARGS' | ||
su $_REMOTE_USER -c 'code-server --bind-addr "$HOST:$PORT" $ARGS "$CODE_SERVER_WORKSPACE"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does ARGS come from and what does it contain? I wonder if we need to worry about quoting and/or newlines?
Right now a '
, "
or \n
would/could break the su command, example:
root@5110bc305062:/# su root -c 'echo hi
>
> there'
hi
bash: -c: line 2: syntax error near unexpected token `newline'
bash: -c: line 2: `>'
entrypoint=$(cat /usr/local/bin/code-server-entrypoint) | ||
check "code-server workspace" grep $'\'code-server.*"/home"\'' <<<"$entrypoint" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to verify more of the cmdline? Ports, args, etc?
Alt suggestion (to useless use of cat 😼):
entrypoint=$(cat /usr/local/bin/code-server-entrypoint) | |
check "code-server workspace" grep $'\'code-server.*"/home"\'' <<<"$entrypoint" | |
check "code-server workspace" grep $'\'code-server.*"/home"\'' < /usr/local/bin/code-server-entrypoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with dropping ARGS
btw, but expanded that discussion a bit. Approving since removing ARGS would be fine, keeping them as-is makes it a bit easy to break the command.
src/code-server/install.sh
Outdated
cat > /usr/local/bin/code-server-entrypoint \ | ||
<< EOF | ||
#!/usr/bin/env bash | ||
set -e | ||
|
||
su $_REMOTE_USER -c 'code-server --bind-addr "$HOST:$PORT" \$ARGS' | ||
su $_REMOTE_USER -c 'code-server --bind-addr "$HOST:$PORT" $ARGS "$CODE_SERVER_WORKSPACE"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, maybe we could use eval for simplicity?
> ARGS="--foo \"some thing\" --'bar'"
> eval "declare -a args=($ARGS)"
> declare -p args
declare -a args=([0]="--foo" [1]="some thing" [2]="--bar")
Used like this:
code-server "${args[@]}"
You can still do something sneaky, like:
ARGS="$(rm -rf /)"
But I don't know if we need care? Someone could just as well include a malicious feature? 🤷🏻♂️
The alternative is that we encode supported code-server flags/opts as explicit feature options, which lets us drop ARGS.
Allow specifying where code-server should be launched into.