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

v2.1.1 #177

Merged
merged 18 commits into from Aug 24, 2022
Merged

v2.1.1 #177

merged 18 commits into from Aug 24, 2022

Conversation

JJ-8
Copy link
Collaborator

@JJ-8 JJ-8 commented Aug 21, 2022

Let's update the main branch so it will have the latest Hedgedoc included, as well as some small fixes.
Since there are no major features, I think v2.1.1 is fine.

XeR and others added 13 commits May 29, 2022 15:29
0 vulnerabilities found - Packages audited: 474
Done in 0.50s.
3 vulnerabilities found - Packages audited: 1501
Severity: 2 Moderate | 1 High

These are caused by eslint-plugin-graphql being deprecated since 2022-01-25.
See: https://github.com/apollographql/eslint-plugin-graphql

Had to replace quasar with quasar-cli
See: quasarframework/quasar#12887 (comment)
There has been a bunch of supply chain attacks in the last few months.

This commit pins every containers to a specific hash (the most recent at the
time) to reduce the risk of CTFNote users pulling a compromised container.
API uses the same container 3 times.
This commit specifies the version of the container in a variable that gets
reused in the Dockerfile. This makes sure we don't forget to update any of the
containers.
Upgrade Hedgedoc to version 1.9.4
The old hash pointed to a non-existing container.
I have updated it to the correct hash:
https://quay.io/repository/hedgedoc/hedgedoc/manifest/sha256:8df47646f2c58959921d2b7719a6f3023441bdb7435040aa3c32abe6145095f6
for both production and development.

Moreover, the api did not work due to an outdated version of ts-node and typescript.
This could be due to running a different node version locally on my machine.
However, running it in Docker now also works.
Now the task dropdown is also visible when you have your browser
resized to half your screen size.
With usernames of normal length, this does not cause an overflow
of the menu bar.
Current Hedgedoc containers are pinned to a specific hash.
Unfortunately, it looks like the Hedgedoc ops remove containers when they update
it.

https://quay.io/repository/hedgedoc/hedgedoc?tab=history
@JJ-8 JJ-8 requested a review from XeR August 21, 2022 17:14
@XeR
Copy link
Contributor

XeR commented Aug 21, 2022

Good idea. I'd like to upgrade dependencies and fix vulnerabilities in dependencies before publishing a new version.

I can't seem to be able to upgrade graphql-upload.
We can upload the package to version 15 which fix the vulnerable dependency.

They changed the way the module is imported, so we need to change src/index.ts:

diff --git a/api/src/index.ts b/api/src/index.ts
index 2b01cf7..b286f47 100644
--- a/api/src/index.ts
+++ b/api/src/index.ts
@@ -2,7 +2,7 @@ import simplifyPlugin from "@graphile-contrib/pg-simplify-inflector";
 import PgPubsub from "@graphile/pg-pubsub";
 import crypto from "crypto";
 import express from "express";
-import { graphqlUploadExpress } from "graphql-upload";
+import graphqlUploadExpress from "graphql-upload/graphqlUploadExpress.mjs";
 import {
   makePluginHook,
   postgraphile,

But then, yarn will complain about types.
One of the issues (jaydenseric/graphql-upload#282) talk about this specific issue.
The fix is to change an option in tsconfig, but it breaks other dependencies.

I tried this, but that doesn't seem to solve the issue :

diff --git a/api/tsconfig.json b/api/tsconfig.json
index 012107d..7d4b405 100644
--- a/api/tsconfig.json
+++ b/api/tsconfig.json
@@ -71,5 +71,11 @@
   },
   "ts-node": {
     "files": true
+  },
+  "graphql-upload": {
+    "compilerOptions": {
+      "maxNodeModuleJsDepth": 10,
+      "module": "nodenext"
+    }
   }
 }

Feel free to give it a try.

As for the front-end, there are two dependencies with vulns : @quasar-cli with no upgrades and eslint-plugin-graphql that appears to be replaced with @graphql-eslint/eslint-plugin -- but I did not manage to upgrade it either.
I will try harder tomorrow.

On a different topic, since the development pace slowed down, should we keep using a dev branch ? Wouldn't it be better for us to merge directly to main and add a tag for new versions ?
This would make pull requests less confusing and dependabot actually useful.

XeR and others added 3 commits August 22, 2022 20:45
6 vulnerabilities found - Packages audited: 1422
Severity: 4 Moderate | 2 High
1 vulnerabilities found - Packages audited: 413
Severity: 1 High

See comments on PR #177
@JJ-8
Copy link
Collaborator Author

JJ-8 commented Aug 23, 2022

@XeR, I think the vulnerable (dev) dependencies are not a big issue for CTFNote.

On a different topic, since the development pace slowed down, should we keep using a dev branch ? Wouldn't it be better for us to merge directly to main and add a tag for new versions ?
This would make pull requests less confusing and dependabot actually useful.

I think this is a good idea. But then we need to update CONTRIBUTING.md since we state there that the target should be the dev branch.
Also, what are we going to do with the dev branch? Delete it? Or leave it and it will become older than main branch?

Force dicer to upgrade to version 0.3.1 by editing the package lock by hand.
This protects against CVE-2022-24434.
Copy link
Contributor

@XeR XeR left a comment

Choose a reason for hiding this comment

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

LGTM (tested with CVE-2022-24434 fix)

@XeR
Copy link
Contributor

XeR commented Aug 23, 2022

But then we need to update CONTRIBUTING.md since we state there that the target should be the dev branch.

Fair point. We'll send a PR to main once this one is merged.

Also, what are we going to do with the dev branch? Delete it? Or leave it and it will become older than main branch?

Yes. I think it does not make sense to keep the dev branch if we do not use it any more.
We can always create a new dev branch in the future if we ever need it.

Ideally I'd like to :

  1. get this PR merged
  2. change CONTRIBUTING.md
  3. make a 2.1.1 tag on main
  4. update both ical PR to point to main
  5. delete dev

Did I forget anything ?

@JJ-8
Copy link
Collaborator Author

JJ-8 commented Aug 24, 2022

I think everything is covered with your list.

@JJ-8 JJ-8 merged commit bae3690 into main Aug 24, 2022
JJ-8 added a commit to JJ-8/CTFNote that referenced this pull request Aug 24, 2022
XeR pushed a commit that referenced this pull request Aug 24, 2022
@XeR XeR deleted the dev branch August 24, 2022 18:49
@JJ-8
Copy link
Collaborator Author

JJ-8 commented Aug 24, 2022

@XeR, I think you missed something your list: I prefer to also publish a Github release (https://github.com/TFNS/CTFNote/releases) after tagging the version. Do you agree?

@XeR
Copy link
Contributor

XeR commented Aug 24, 2022

Oh that's nice. I had no idea we had those !
I put the releases link back on, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants