feat: Use new Codecov uploader #392
Conversation
|
Is this PR intended to consume http://github.com/codecov/uploader? |
|
@RA80533 if you have any suggestions on what is actually going wrong here with running |
|
Was there a change made to codecov-linux some time today? It appears that the error originates from it. |
Codecov Report
@@ Coverage Diff @@
## master #392 +/- ##
==========================================
+ Coverage 96.45% 97.39% +0.93%
==========================================
Files 3 3
Lines 141 115 -26
Branches 43 34 -9
==========================================
- Hits 136 112 -24
+ Misses 5 3 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
It might be helpful to try to migrate this script into a local clone of codecov/uploader to run directly against its source. You might end up with an exact line number for the syntax error compared to what appears to be one produced from the minified source code. You could translate the TypeScript directly to a JavaScript file to run it directly with Node.js or use the ts-node package to run transpile and run the TypeScript directly. Additionally, since GitHub Actions runs this in its own containerized environment wherein application dependencies such as Node.js are available, it would be possible to leverage the uploader project directly in JavaScript rather than through the binary it produces. Both projects are written with the same language and platform technologies so one would be able to consume code from the other (pre-build, of course) through package publication mechanisms available from the npm registry or Github Packages, etc. |
|
Remember to install both the current project dependencies and those of the now-nested one. It appears that the installation process requires there to be a Git repository in its folder which causes issues for installation (due to the requirement of having Husky hooks, etc.). The following change allowed me to install the project without a hitch: diff --git a/uploader/package.json b/uploader/package.json
index 8ce71cb..14661a8 100644
--- a/uploader/package.json
+++ b/uploader/package.json
@@ -10,8 +10,7 @@
"build-linux": "pkg . --targets linux --output out/codecov-linux",
"build-macos": "pkg . --targets macos --output out/codecov-macos",
"build-alpine": "pkg . --targets node14-alpine-x64 --output out/codecov-alpine",
- "build-windows": "pkg . --targets win --output out/codecov.exe",
- "prepare": "husky install"
+ "build-windows": "pkg . --targets win --output out/codecov.exe"
},
"repository": {
"type": "git",
In other words, simply remove the |
|
I enabled the diff --git a/tsconfig.json b/tsconfig.json
index dbe3a5a..fa348fc 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -1,4 +1,7 @@
{
+ "compilerOptions": {
+ "sourceMap": true
+ },
"include": [
"src"
]
|
|
Based on the workflow file used in the GitHub Actions runner, main.yml: $ INPUT_FILES='./coverage/calculator/coverage-final.json,./coverage/coverage-test/coverage-final.json' \
> INPUT_FILE='./coverage/coverage-final.json' \
> INPUT_FLAGS='demo' \
> INPUT_NAME='codecov-demo' \
> node --enable-source-maps dist/index.js
TypeError: Cannot read property 'url' of undefined
at Object.main (/[…]/codecov-action/dist/index.js:25391:55)
at /[…]/codecov-action/dist/index.js:25814:14
at /[…]/codecov-action/dist/index.js:25821:3
at Object.<anonymous> (/[…]/codecov-action/dist/index.js:25824:12)
at Module._compile (node:internal/modules/cjs/loader:1095:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
at Module.load (node:internal/modules/cjs/loader:975:32)
at Function.Module._load (node:internal/modules/cjs/loader:816:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
at node:internal/main/run_main_module:17:47Interestingly, it appears that disabling Source Map v3 support (e.g., not passing the $ INPUT_FILES='./coverage/calculator/coverage-final.json,./coverage/coverage-test/coverage-final.json' \
> INPUT_FILE='./coverage/coverage-final.json' \
> INPUT_FLAGS='demo' \
> INPUT_NAME='codecov-demo' \
> node dist/index.js
/[…]/codecov-action/dist/index.js:25391
const uploadHost = validateHelpers.validateURL(args.url)
^
TypeError: Cannot read property 'url' of undefined
at Object.main (/[…]/codecov-action/dist/index.js:25391:55)
at /[…]/codecov-action/dist/index.js:25814:14
at /[…]/codecov-action/dist/index.js:25821:3
at Object.<anonymous> (/[…]/codecov-action/dist/index.js:25824:12)
at Module._compile (node:internal/modules/cjs/loader:1095:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
at Module.load (node:internal/modules/cjs/loader:975:32)
at Function.Module._load (node:internal/modules/cjs/loader:816:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
at node:internal/main/run_main_module:17:47 |
|
@RA80533 we're getting closer 🙏 |
diff --git a/tsconfig.json b/tsconfig.json
index dbe3a5a..99ea5db 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -1,5 +1,13 @@
{
+ "compilerOptions": {
+ "outDir": "./dist",
+ "rootDir": "./src",
+ "sourceMap": true
+ },
"include": [
- "src"
+ "src/**/*.ts"
+ ],
+ "exclude": [
+ "src/**/*.test.ts"
]
}
With
|
|
I'm confused. Why are we embedding the uploader? Doesn't that nullify all the binary security? |
|
@drazisil I'm not sure so correct me if I'm wrong, but from an engineering perspective, I don't think we need all the indirection. We get the advantage of having |
|
There was a "this error exists on line 1 because everything is on line 1" error due everything being packed into the binary. Importing it allowed the error to become more apparent due to the additional context available from the source code. |
I might be able to shed some light on this perspective. Are there any other mechanisms in place aside from the checksums? |
No, but part of the reason for making it a binary was so that folks couldn't use it as a package and thus possibly bypass the validation by calling upload directly, for example. In that sense, being a binary is part of the security, in the sense it's much harder to tamper with or use incorrectly. Also if it's bundled like this it's not current and we now have multiple versions running around. |
You get a version that becomes out of date as soon as we update the uploader. |
|
I've invested too much time and pain to get it working as a binary to objectively be ok with bypassing that because "node is easier". If product is ok with this approach, then I'm fine. 🤷♀️😁 |
Yes, with the way the uploader is currently embedded, it will immediately go out of date and require repository-level maintenance in order to bring it up to date. The problem its current embedding attempts to solve is somewhat of a classical problem in software architecture. Fortunately, it can be solved by approaching the problem a bit differently with the context of the technologies in use:
The binary itself should not be thought of as a security measure. Although it is a very common practice (it's even got a name: "security through obscurity"), it is somewhat misleading because it does not prevent the mechanisms through which exploitation would be possible. The practice has legitimate application when used alongside proven security measures, however, and can be quite effective with dealing with automated scanning and the like. The binary itself has tremendous value in its portability. The Bash uploader had a handful of dependencies outside of its control that no doubt made it tricky to work with in certain environments. Those requirements are nowhere to be found in the new uploader. |
I respectfully disagree. This is not security though obscurity, in the sense that it's an obficated script. This is a true binary, and as such I believe it blocks quite a few ways to tamper with it that are possible with a node script. |
|
Thanks for the discussion here. I think I'd rather us be more security conscious and go with the older version of this PR (downloading, validating, and running the binary). If it ends up being unnecessary, we can make that call later. But I think it would be worse to do it the other way around. Unless of course, this is the WRONG way to publish this action securely |
Simply for clarification, are you referring to a scenario in which an attacker modifies an executable that is later run by an unknowing party or the attacker themselves?
Be aware that the earlier version of the PR was susceptible to the same attack vector as the issue from earlier this year. One improvement upon which the process you described can take on is for it to download the binary from a place where changes made to it are transparently available for users to see. |
This is, somewhat surprisingly, not too difficult to accomplish.
|
|
Fixing the uploader via codecov/uploader#200 |
thomasrockhu
force-pushed
the
v2
branch
2 times, most recently
from
7dae0eb
to
ec2dbcb
Compare
jest.config.js
Outdated
| testPathIgnorePatterns: [ | ||
| '/node_modules/', | ||
| ], |
There was a problem hiding this comment.
Jest should ignore this by default.
|
Even though this is written in TypeScript, there may be difficulty in obtaining type definitions during development due to a specific caveat with Node.js: If you're using an editor with first-class TypeScript capabilities such as VS Code, you can leverage editor hinting for the types you're interacting with by making a subtle change to module imports: - const fs = require('fs');
+ import * as fs from 'fs';With the enablement of the |
src/index.ts
Outdated
| // TODO - validate step | ||
|
|
||
| fs.chmodSync(filename, '777'); | ||
| exec.exec(filename, execArgs, options) |
There was a problem hiding this comment.
This should be awaited via the await keyword. Two things to note:
- The enclosing function should be made async on line 44
- Lines 61 to 70 should be moved before line 51 to prevent a TypeError from being thrown
| exec.exec(filename, execArgs, options) | |
| await exec.exec(filename, execArgs, options) |
There was a problem hiding this comment.
Nice, makes sense. For my future reference, how did you realize this was an issue?
There was a problem hiding this comment.
Good question.
I was able to discover this due to looking at the imports and realizing two were still brought in via require() instead of import. This led me to understand that the type definitions of those modules were not brought in and available to the developer. Once they were, it became directly apparent that exec() was typed to return a Promise<number>.
There was a problem hiding this comment.
An unfortunate side effect when dealing with promises is that it's quite easy for them to "dangle" if their settlement is never handled. TypeScript does not alert for this but Node.js will emit an error message indicating an UnhandledRejection has occurred if a dangling promise is rejected / throws an error.
There was a problem hiding this comment.
There's a TSLint rule (no-floating-promises) that checks for this behavior. ESLint does not have a built-in rule that corresponds with this behavior although there are a few from the community (e.g., the one from the typescript-eslint project) that handles it well.
|
Since there are commits on this PR that are not signed, I will need to do a rebase which will be destructive to this PR |
|
Sorry all, this PR has gotten a little out of hand, I'm going to squash it down to the original commit before the rebase |
thomasrockhu
force-pushed
the
v2
branch
3 times, most recently
from
f67cedb
to
12b131e
Compare
Bumps [eslint](https://github.com/eslint/eslint) from 7.27.0 to 7.28.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md) - [Commits](eslint/eslint@v7.27.0...v7.28.0) --- updated-dependencies: - dependency-name: eslint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Due to the deprecation of the bash uploader, this PR pushes the Action to v2 with the new uploader.