Implemented Containerization of the Lyra Webapp #157

henrycatalinismith · 2025-02-01T17:38:55Z

Description

Added containerization to the lyra project to be able to run the whole thing inside of a container.
Read through the Docker setup section within the README.md for more details on how this works.

xela1601 · 2025-02-02T10:03:34Z

Copying over @henrycatalinismith 's PR description here to not override it:

Not fully working yet but lots of annoying obstacles cleared so far. Here's the error message as of the end of the day.

--- ~/lyra ‹main* M› » make docker_run
docker run -it \
		-v /Users/henrycatalinismith/lyra/lyra/config/docker.yaml:/app/config/projects.yaml \
		-v /Users/henrycatalinismith/lyra/test-001:/projects/test-001 \
		-p 3000:3000 \
		lyra
  ▲ Next.js 14.2.15
  - Local:        http://769f01fc1fc2:3000
  - Network:      http://172.17.0.2:3000

 ✓ Starting...
 ✓ Ready in 64ms
loading
a [Error]: unable to fork
    at Object.onError (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:44708)
    at /app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:18797
    at new Promise (<anonymous>)
    at W.handleTaskData (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:18556)
    at W.<anonymous> (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:18181)
    at Generator.next (<anonymous>)
    at o (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:4300)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  task: {
    commands: [ 'pull' ],
    format: 'utf-8',
    parser: [Function: parser],
    onError: [Function: onError]
  },
  git: ef {
    remote: '',
    hash: { local: '', remote: '' },
    branch: { local: '', remote: '' },
    message: 'unable to fork'
  },
  digest: '4075981265'
}
a [Error]: unable to fork
    at Object.onError (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:44708)
    at /app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:18797
    at new Promise (<anonymous>)
    at W.handleTaskData (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:18556)
    at W.<anonymous> (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:18181)
    at Generator.next (<anonymous>)
    at o (/app/webapp/.next/standalone/webapp/.next/server/chunks/871.js:26:4300)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  task: {
    commands: [ 'pull' ],
    format: 'utf-8',
    parser: [Function: parser],
    onError: [Function: onError]
  },
  git: ef {
    remote: '',
    hash: { local: '', remote: '' },
    branch: { local: '', remote: '' },
    message: 'unable to fork'
  },
  digest: '4075981265'
}

xela1601 · 2025-02-02T13:03:01Z

there's still some issue with the unit tests that needs to be addressed

xela1601 · 2025-02-02T13:18:16Z

Also i just noticed that the publishing of translation changes is not working at the moment.

 Error while creating pull request: Error: fatal: ../zetkin-fork-for-lyra/src/locale/da.yml: '../zetkin-fork-for-lyra/src/locale/da.yml' is outside repository at '/app/zetkin-fork-for-lyra'

This is some weird issue with the paths that are being used throughout the webapp.
I'd propose we just configure all the paths being used in one central location...

What do you think?

WULCAN · 2025-02-02T14:08:18Z

Also i just noticed that the publishing of translation changes is not working at the moment.
 Error while creating pull request: Error: fatal: ../zetkin-fork-for-lyra/src/locale/da.yml: '../zetkin-fork-for-lyra/src/locale/da.yml' is outside repository at '/app/zetkin-fork-for-lyra'
This is some weird issue with the paths that are being used throughout the webapp. I'd propose we just configure all the paths being used in one central location...

What do you think?

That looks like a bug in Lyra. If I recall correctly, we convert to absolute paths pretty early after reading projects.yaml but we must have made a mistake somewhere. Will you create a separate issue for this bug?

I'm not sure exactly what you are suggesting with configuring all paths in a central location. Our configuration file projects.yaml could be considered that already.

WULCAN

I've reviewed all the changes and leave my notes here. I haven't done any testing yet.

I haven't concluded what needs fixing and what can be deferred to later, except for the one about copying config into the container image. That one seems like a serious issue to me.

Dockerfile

webapp/src/utils/serverConfig.ts

Signed-off-by: Alexander Schreiner <[email protected]>

since it contains the github token which should not be inside of the image Signed-off-by: Alexander Schreiner <[email protected]>

- improved logging - introduced dynamic .yml and .yaml support for configuration - added host projects config - only copying webapp folder into builder stage - bumping up to node version 23 - adding git username/password to docker env variables - adjusted README Signed-off-by: Alexander Schreiner <[email protected]>

Signed-off-by: Alexander Schreiner <[email protected]>

Dockerfile

WULCAN · 2025-02-23T07:45:13Z

webapp/next.config.js

@@ -1,4 +1,6 @@
 /** @type {import('next').NextConfig} */
-const nextConfig = {};
+const nextConfig = {
+  output: 'standalone',


With standalone output, Next can miss required dependencies from the monorepository root. Next documents that we need to configure outputFileTracingRoot to work around this.

https://nextjs.org/docs/14/app/api-reference/next-config-js/output#caveats

While our monorepository root only has devDependencies, one of those dependencies is typescript, which is actually a runtime dependency of webapp:

lyra/webapp/src/utils/readTypedMessages.ts

Line 1 in 83f84ef

import ts from 'typescript';

Somewhat fortunately, we observe that standalone contains the required typescript in node_modules and we can also see that it includes the monorepository root package.json. Note that we get a webapp/server.js instead of the expected server.js. My interpretation is that Next has an undocumented feature, or perhaps a bug, making Next consider the whole monorepository, even when next build is run from inside the subproject.

vercel/next.js#72436

It's unclear what will happen if we follow the documentation by configuring outputFileTracingRoot when Next already has behaviour of considering the monorepository. I think either way, there's risk of our setup breaking when we upgrade Next or when we add more projects to our monorepository.

Resolving this upstream with Next is likely going to be slow, given how the issue linked above was handled.

If we could add a comment near our dependency declaration for Next, we could warn an upgrading developer. I do not believe it is possible to add comments near the dependency declaration in package.json. What we can do instead is add a section about it to README. There's a low probability of that being read by an upgrading developer but it will still be a lot more visible than a discussion in an old pull request.

@WULCAN

i was just wondering why we need this webapp module within the monorepo anyway.

Or to ask the question in another way: why can't we just have one package.json and merge all the stuff from webapp into the repository root 🤔

This would definitely fix that issue but will require some more changes.
Either way it would be good to know what was the reason for this decision in the first place.

@WULCAN
i adjusted the next.config.js and updated the README accordingly in 1e781b5

Unfortunately, the conversations about #35 have been lost in slack history.

I believe we have some ideas about a future Lyra library and other Lyra tools than the web application. In that future, a shared version control repository for these different projects might be useful. @amerharb might remember more.

docker-compose.yaml

webapp/README.md

.dockerignore

docker-compose.yaml

Dockerfile

webapp/src/utils/paths.ts

henrycatalinismith · 2025-05-25T10:44:15Z

amerharb · 2025-05-25T12:38:48Z

webapp/src/utils/serverConfig.ts

+          return new ServerProjectConfig({
+            baseBranch: project.base_branch ?? 'main',
+            githubToken: project.github_token,
+            host: project.host,


suggest to have a default value of github.com in the same way that we have main to baseBranch

Fixed in 786feed @amerharb

WULCAN · 2025-05-25T13:34:05Z

I have reviewed Henry's a2ff94a , 97e8659, bc0b0aa and 5ec7e77 and approve of them.

I also double checked his amazing list of remaining issues above.

The value for the parameter translation is entirely controlled by an unauthenticated client on Internet. Reflecting that into logs allows them to spoof other log message too, for example by using newline characters in the value. It might even give them control over the administrator's terminal using control characters.

Prior to validation, languageName is entirely controlled by an unauthenticated client. Logging its value allows them to inject arbitrary content into logs, including but not limited to spoofed log messages and control characters.

This log line reflects unauthenticated client content into logs. If accessLanguage fails, either Next will log the exception or our warn below will log similar info as this line. If accessLanguage succeeds, our info will log similar info to this removed line.

accessLanguage only returns false-like if the project was not found. Then the languageName is irrelevant. languageName is entirely controlled by an unathenticated client at this point. Logging its value allows arbitrary log injection.

For this log line, the value is entirely controlled by an unauthenticated client. Reflect that into logs allows arbitrary log injection. Hex encoding it is safer, at the expense of readability for the administrator. This error case should be rare though so sacrificing readability is not too expensive. toHex will be moved to a better place when we discover more usages for it.

Signed-off-by: Alexander Schreiner <[email protected]>

…rectory Signed-off-by: Alexander Schreiner <[email protected]>

Resolve some log injection from #157

Signed-off-by: Alexander Schreiner <[email protected]>

WULCAN · 2025-05-29T09:49:25Z

Note that:

5a929c5 was pushed with 5206e0b as parent but later replaced by e052c82 with a force push.
7dff829 was pushed with c15aa47 as parent but then replaced by f497ccb with a force push.

WULCAN · 2025-05-27T17:42:47Z

package-lock.json

My npm 10.9.2 cannot reproduce the package-lock.json of 607027a from the corresponding package.json. That doesn't mean its wrong, but I think we need to understand why it's different. What tool did you use and how?

WULCAN · 2025-05-29T08:35:12Z

webapp/package.json

@@ -21,7 +21,7 @@
    "@mui/material-nextjs": "^5.16.8",
    "@mui/x-tree-view": "^7.23.0",
    "@octokit/rest": "^20.1.1",
-    "next": "14.2.15",
+    "next": "^14.2.29",


We use two modules form the same project: next and eslint-config-next. The Next project expects them to be in sync.

lyra/package.json

Line 32 in f497ccb

"eslint-config-next": "14.2.15",

Actually, we also depend on a really fishy eslint-plugin-next 0.0.0 but let's handle that in a separate issue: #197

README.md

WULCAN · 2025-05-29T09:04:21Z

webapp/next.config.js

+
+  // 👇 make tracing start at the monorepo root
+  experimental: {
+    outputFileTracingRoot: path.join(__dirname, '../..'),


../.. from webapp is outside of our monoreposiroty root. The example from Next.js docs is for a package in packages/web-app. Our package is just in webapp

See also https://github.com/zetkin/lyra/blob/f497ccb752d3e2ac384b036647605285e4c67d88/README.md#standalone-build-monorepo

WULCAN · 2025-05-29T09:28:15Z

README.md

+
+  ```bash
+  npm run build
+  node webapp/.next/standalone/lyra/webapp/server.js --check


Let's move --check to before the script, to make sure and clear that it's a flag to node, and not not to server.js

WULCAN · 2025-05-29T09:29:07Z

README.md

+
+  ```bash
+  npm run build
+  node webapp/.next/standalone/lyra/webapp/server.js --check


The flag --check tells Node to check the script's syntax without executing it. Does this really help to catch missing dependencies?

webapp/src/utils/serverConfig.ts

WULCAN · 2025-05-29T10:23:08Z

webapp/src/RepoGit.ts

+    /*
+    check if directory at repoPath contains something.
+    if it does, we skip cloning
+    if it does not, we clone the repo there since
+    "Cloning into an existing directory is [...] allowed if the directory is empty." according to git docs:
+    https://git-scm.com/docs/git-clone#Documentation/git-clone.txt-emltdirectorygtem
+    */
+    if (repoFolderExists) {
+      const content = fs.readdirSync(spConfig.repoPath);
+      if (content.length !== 0) {
+        info(
+          `Repo folder at ${spConfig.repoPath} exists but is not empty, skipping clone.`,
+        );
+        return;
+      }
+    }
+
    if (!repoFolderExists) {
+      info(`Cloning repo because it does not exist at ${spConfig.repoPath}`);
      await RepoGit.clone(spConfig);
    }


If repoFolderExists, !repoFolderExists is not and we will not clone due to the branching at line 79.

WULCAN · 2025-05-29T10:53:26Z

webapp/src/utils/serverConfig.ts

+            owner: project.owner,
+            projectPath: path.normalize(project.project_path),
+            repo: project.repo,
+            repoPath: path.resolve(paths.lyraProjectsAbsPath, project.repo),


Consider an administrator setting up Lyra as a translation management system for two projects A/X and B/X.

She creates a projects.yaml with the following content:

projects: - name: A base_branch: main project_path: . owner: A repo: X host: github.com github_token: << github token >> - name: B base_branch: main project_path: . owner: B repo: X host: github.com github_token: << github token >>

When Lyra parses this in serverConfig.ts, Lyra will create two ServerProjectConfig with the same repoPath: lyra-projects/X.

As Lyra is used, Lyra will create two instances of RepoGit with the same repoPath. Probably, Lyra will only clone one of them, and use that for both projects.

There might be other severe concurrency issues too, consider for example that simple-git's clone is async, or that different projects can have different base_branch. This might also be a problem for monorepos where both owner and repo are the same but project_path differs. Basically, the previous implementation with separate repo_path for each project was a lot safer and easier to reason about.

For this pull request, let's just add a warning in README to the administrator that repo must not have the same value for two different projects.

henrycatalinismith force-pushed the issue-156/dockerfile branch from d09ed15 to 71486b6 Compare February 1, 2025 17:39

xela1601 force-pushed the issue-156/dockerfile branch from 71486b6 to 0eaa070 Compare February 2, 2025 08:35

xela1601 changed the title ~~Dockerfile progress for the day~~ Implemented Containerization of the Lyra Webapp Feb 2, 2025

xela1601 closed this Feb 2, 2025

xela1601 force-pushed the issue-156/dockerfile branch from 4801da3 to 7c8e25b Compare February 2, 2025 11:31

xela1601 reopened this Feb 2, 2025

xela1601 marked this pull request as ready for review February 2, 2025 11:34

xela1601 force-pushed the issue-156/dockerfile branch from 790557d to ed66bc6 Compare February 2, 2025 11:48

WULCAN reviewed Feb 2, 2025

View reviewed changes

xela1601 force-pushed the issue-156/dockerfile branch 2 times, most recently from 6d4350b to 38613d7 Compare February 2, 2025 19:56

xela1601 force-pushed the issue-156/dockerfile branch 3 times, most recently from dc564fe to 23d3297 Compare February 15, 2025 21:13

henrycatalinismith and others added 6 commits February 15, 2025 22:35

giving up for the day

29c17c3

Fixing the docker setup

279e863

Signed-off-by: Alexander Schreiner <[email protected]>

adding gitlab public key to known host

aa6ad88

Signed-off-by: Alexander Schreiner <[email protected]>

moving config out of the docker image for security reasons

01cd797

since it contains the github token which should not be inside of the image Signed-off-by: Alexander Schreiner <[email protected]>

clone projects into projects folder in docker and run git config

87d0ad0

xela1601 force-pushed the issue-156/dockerfile branch from 23d3297 to 83f84ef Compare February 15, 2025 21:38

cleanup dockerignore and handling store.json

9d04a5d

Signed-off-by: Alexander Schreiner <[email protected]>

WULCAN requested changes Mar 29, 2025

View reviewed changes

This was referenced Mar 30, 2025

NODE_ENV production #164

Open

Migrate to node image's user #165

Open

Docker with encrypted private key #167

Open

henrycatalinismith added 2 commits May 25, 2025 12:15

Add extra paragraph

97e8659

Remove GPG-related change to commit behaviour

bc0b0aa

henrycatalinismith mentioned this pull request May 25, 2025

Configurable commit flags to e.g. disable GPG #193

Open

Revert error handling to omit potentially sensitive info

5ec7e77

amerharb reviewed May 25, 2025

View reviewed changes

WULCAN added 5 commits May 25, 2025 15:42

Log after validation

570945e

Prior to validation, languageName is entirely controlled by an unauthenticated client. Logging its value allows them to inject arbitrary content into logs, including but not limited to spoofed log messages and control characters.

Remove log Line

f206aae

This log line reflects unauthenticated client content into logs. If accessLanguage fails, either Next will log the exception or our warn below will log similar info as this line. If accessLanguage succeeds, our info will log similar info to this removed line.

Remove languageName from log record

390961a

accessLanguage only returns false-like if the project was not found. Then the languageName is irrelevant. languageName is entirely controlled by an unathenticated client at this point. Logging its value allows arbitrary log injection.

WULCAN mentioned this pull request May 25, 2025

Resolve some log injection from #157 #194

Merged

WULCAN and others added 7 commits May 25, 2025 16:33

Hex encode project name in project page too

8cc8db7

update next.js to resolve critical vulnerability

607027a

Signed-off-by: Alexander Schreiner <[email protected]>

updated browserlist via npx update-browserslist-db@latest

460298d

Signed-off-by: Alexander Schreiner <[email protected]>

fix docker paths within README.md

f46a57c

Signed-off-by: Alexander Schreiner <[email protected]>

add outputFileTracingRoot & document standalone monorepo caveats

1e781b5

Signed-off-by: Alexander Schreiner <[email protected]>

use node 22 in dockerfile

5206e0b

Signed-off-by: Alexander Schreiner <[email protected]>

adjust project_path reference in README

e052c82

Signed-off-by: Alexander Schreiner <[email protected]>

xela1601 force-pushed the issue-156/dockerfile branch from 5a929c5 to e052c82 Compare May 27, 2025 17:40

xela1601 and others added 5 commits May 27, 2025 19:43

make serverConfig's host optional with default value of github.com

786feed

Signed-off-by: Alexander Schreiner <[email protected]>

adjust logic of cloneIfNotExist method to allow cloning into empty di…

6cf337a

…rectory Signed-off-by: Alexander Schreiner <[email protected]>

Merge pull request #194 from zetkin/issue-156/log-injection

98f24ff

Resolve some log injection from #157

make messagesPath in lyraConfig private again

c15aa47

Signed-off-by: Alexander Schreiner <[email protected]>

fix: avoid leaking absolute paths in getProjectsConfigPath

f497ccb

Signed-off-by: Alexander Schreiner <[email protected]>

xela1601 force-pushed the issue-156/dockerfile branch from 7dff829 to f497ccb Compare May 28, 2025 09:02

WULCAN requested changes May 29, 2025

View reviewed changes

WULCAN mentioned this pull request May 29, 2025

Support other forges than GitHub #196

Open

Implemented Containerization of the Lyra Webapp #157

Are you sure you want to change the base?

Implemented Containerization of the Lyra Webapp #157

Uh oh!

Conversation

henrycatalinismith commented Feb 1, 2025 • edited by xela1601 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

xela1601 commented Feb 2, 2025

Uh oh!

xela1601 commented Feb 2, 2025

Uh oh!

xela1601 commented Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WULCAN commented Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WULCAN left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WULCAN May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henrycatalinismith commented May 25, 2025 • edited by WULCAN Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WULCAN commented May 25, 2025

Uh oh!

WULCAN commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WULCAN May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrycatalinismith commented Feb 1, 2025 •

edited by xela1601

Loading

xela1601 commented Feb 2, 2025 •

edited

Loading

WULCAN commented Feb 2, 2025 •

edited

Loading

WULCAN left a comment •

edited

Loading

WULCAN May 27, 2025 •

edited

Loading

henrycatalinismith commented May 25, 2025 •

edited by WULCAN

Loading

WULCAN commented May 29, 2025 •

edited

Loading

WULCAN May 29, 2025 •

edited

Loading