Skip to content

Register credentials extracted from proxy configuration as secrets #2930

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 16, 2025
Merged

Register credentials extracted from proxy configuration as secrets #2930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions lib/start-proxy.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/start-proxy.js.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions src/start-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import * as core from "@actions/core";

import { parseLanguage, Language } from "./languages";
import { Logger } from "./logging";
import { ConfigurationError } from "./util";
Expand Down Expand Up @@ -63,6 +65,14 @@

const out: Credential[] = [];
for (const e of parsed) {
// Mask credentials to reduce chance of accidental leakage in logs.
if (e.password !== undefined) {
Copy link

Choose a reason for hiding this comment

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

Consider testing for null as well to guard against future changes. Better safe than sorry in these cases...

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't disagree, but we don't seem to check for null anywhere else (including elsewhere here where we inspect other members of the same JSON object).

I think we can do a much better job at validating the JSON value we get in general (e.g. check that it actually is an array, rather than just assuming it), but that may be a larger chunk of work. I am not sure if it makes sense to change this one now and not the others - what do you think?

Copy link

Choose a reason for hiding this comment

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

Hmm, good question. I was about to write that it is probably more important to be consistent and make a sweep later with proper validation.

But: What is the impact if the structure of the JSON changes and makes our assumptions false in other places vs. here? In other places something might just crash, which we can hopefully detect and fix, but here we risk leaking secrets.

Your call though, as I haven't looked at the other places, and don't know the severity of the potential leak...

core.setSecret(e.password);

Check failure

Code scanning / CodeQL

Untrusted data passed to external API with additional heuristic sources High Experimental

Call to @actions/core.setSecret() [param 0] with untrusted data from
e.password
Loading
.
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like a FP to me.

}
if (e.token !== undefined) {
core.setSecret(e.token);
}

if (e.url === undefined && e.host === undefined) {
// The proxy needs one of these to work. If both are defined, the url has the precedence.
throw new ConfigurationError(
Expand Down
Loading