feat: added firebase module #2954
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
Thanks for the PR, I've done a initial pass. The majority of comments center around bringing it inline with the latest template for modules in terms of style and correct behaviour e.g. handling non nil container with error.
modules/firebase/firebase.go
Outdated
| // WithRoot sets the directory which is copied to the destination container as firebase root | ||
| func WithRoot(rootPath string) testcontainers.CustomizeRequestOption { | ||
| return func(req *testcontainers.GenericContainerRequest) error { | ||
| if !strings.HasSuffix(rootPath, "/firebase") { |
There was a problem hiding this comment.
question: Could you clarify why this requirement is needed?
There was a problem hiding this comment.
This has been an issue a year ago. Mounting did not correctly work at that time, and if the source directory did not match the destination directory, mounting would nest the directory instead of mapping it directly.
I see that the issue has been fixed, removing.
There was a problem hiding this comment.
Ah, my bad. The issue still exits. I wrote a failing test to exhibit the issue.
Is there something incorrect with the way I copy over the configuration?
Also, I don't think that volume mount would work in this case, because firebase emulator creates a lot of trash data in the root catalog, which in some cases can even make testing flaky (especially if using DATA_DIRECTORY as a fixture).
There was a problem hiding this comment.
I'll try to find some time to look. I had a some in progress work to fix up the copy behaviour which was odd.
If you have a test to exercise this that, would help alongside a description of the desired behaviour and what you're seeing instead.
modules/firebase/ports.go
Outdated
| return fmt.Sprintf("%s:%s", host, port.Port()), nil | ||
| } | ||
|
|
||
| func (c *FirebaseContainer) UIConnectionString(ctx context.Context) (string, error) { |
There was a problem hiding this comment.
question: seems like lots of additional API surface area instead of just exposing ConnectionString, thoughts?
If not then we'd need comments for all the methods.
There was a problem hiding this comment.
This one is a bit more tricky.
I would love to change all of these methods, and figure out a better way to configure the launched container.
Currently I ship a catalog which contains static predefined configuration for the emulator. These methods happen to match the port numbers defined in the said file.
But, in the tests that we use, we provide a completely different setup (because we don't want to boot the full emulation suite), and we have to match the port numbers in our config so that all of this works.
So a question from my side would be:
Is there a way to generate file based config during container setup? To my knowledge firebase emulator does not allow passing those values as environment variables.
I think, ideal use example would be this:
firebaseContainer, err := firebase.Run(ctx,
"ghcr.io/u-health/docker-firebase-emulator:13.29.2",
firebase.WithUI(),
firebase.WithFirestore(),
firebase.WithCache(),
)And then, helper methods XXXConnectionString would actually return meaningful errors if the emulator part was not launched.
There was a problem hiding this comment.
Is there a way to generate file based config during container setup?
@xytis you can take a look at the redpanda module, where there is a Go template the module is using to build the configuration file.
There was a problem hiding this comment.
After some testing on my side, I remembered why I shipped the config as a directory in the first place.
Default (and probably expected) use case for firebase projects, is to run firebase init and then use the scaffolded configuration to interact with the dependencies.
In the following commit, I've changed the behaviour of this module. It now attempts to understand the emulator configuration of your project and then boots the containerised emulator with that info.
At least in my use case, that makes sense, and when I launch the tests, I use the same project-wide config for all of them.
modules/firebase/ports.go
Outdated
| func (c *FirebaseContainer) connectionString(ctx context.Context, portName nat.Port) (string, error) { | ||
| host, err := c.Host(ctx) | ||
| if err != nil { | ||
| return "", err |
There was a problem hiding this comment.
suggestion: wrap the error so the user knows the cause, more below.
xytis
force-pushed
the
main
branch
2 times, most recently
from
7404470 to
cc2bfa4
Compare
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
I added a few minor comments, we are in the good way.
Thanks for your contribution
|
|
||
| ### Container Methods | ||
|
|
||
| The Firebase container exposes the following methods: |
There was a problem hiding this comment.
docs: we probably need to append here the new methods the container expose
There was a problem hiding this comment.
@xytis let's not forget adding here ConnectionString 🙏
| E.g. `Run(context.Background(), "ghcr.io/u-health/docker-firebase-emulator:13.29.2")`. | ||
|
|
||
| {% include "../features/common_functional_options.md" %} | ||
|
|
There was a problem hiding this comment.
docsd: we need to add here all the functional options for the module.
There was a problem hiding this comment.
@xytis let's not forget adding the WithData option here 🙏
|
One question: reading the README in the docker image:
What are the differences with the existing Google Cloud emulators? We do have them under the gcloud module, and we currently plan to follow the same package structure as in the upcoming azure module (see #3008). Do you think this firebase has enough entity to be a separate module, or should it live alongside the other gcloud modules? |
|
Last time I checked, gcloud emulators did not provide the full suite of emulations. I will check this again and come back to you. |
|
@xytis we merged the layout revamp in the gcloud module. Do you think we can move this there? |
|
Hey, unfortunately not.
I am not certain about the rest. |
but we're going to use the I do not have an opinion on this, so as the author of the image, please feel free to be prescriptive about it. 🙏 |
|
Well, if I was searching for "firebase" emulators, I would not look under "gcloud". Mostly because the tooling and the documentation (both provided by google) are very different for these two products. I think, having these two separate makes more sense as long as there are development differences between running |
|
LGTM then to continue as a separate module, thanks! 🙏 I can help you out rebasing this PR resolving the conflicts before moving on with the review |
|
Please do, I am a bit occupied at the moment. Thanks. |
|
@xytis I cannot push to your branch because this PR was submitted from your main branch 😞 I've pushed it to my origin so that you can check and pick up the commits: https://github.com/testcontainers/testcontainers-go/compare/main...mdelapenya:testcontainers-go:firebase-module?expand=1 I still have doubts about the |
|
@mdelapenya merged your changes. I was forced to use WithData because Firebase emulators expect a specific directory structure and files to exist in the "project root". I toyed with providing structured config, but I am not using that in my own tests. It was easier to bootstrap a directory with a specific catalog structure, and I just clone that into the container. Rough docs on this are here: https://firebase.google.com/docs/emulator-suite/install_and_configure |
|
@xytis lint is complaining. Could you run |
|
|
||
| go 1.23.0 | ||
|
|
||
| require ( |
There was a problem hiding this comment.
suggestion: just before submitting the changes, run go mod tidy from the module dir to make sure all dependencies are in sync with the core. Else we'll receive an extra PR from dependabot 😅
There was a problem hiding this comment.
go mod tidy returned no changes. Was this expected?
There was a problem hiding this comment.
Probably no changes are missing. I just want to double check we are not missing recent updates in the main branch.
|
@xytis for some reason, the lint still fails: Once fixed, and the docs are updated (see #2954 (comment) and #2954 (comment)), I think we are good to merge. |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
I'm approving this PR already, although there are some comments to be addressed before we merge it:
- documentation adding the WithData options and the ConnectionString container method
- Steven's suggestion regarding wrapping errors.
Other than that, LGTM, thanks so much for your patience during the review, we do appreciate your hard work with Testcontainers and the Firebase emulator. 🙇
modules/firebase/ports.go
Outdated
| func (c *Container) ConnectionString(ctx context.Context, portName nat.Port) (string, error) { | ||
| host, err := c.Host(ctx) | ||
| if err != nil { | ||
| return "", err |
There was a problem hiding this comment.
| return "", err | |
| return "", fmt.Errorf("host: %w", err) |
There was a problem hiding this comment.
I'll get to this once I have some time :)
There was a problem hiding this comment.
I'm cutting a release today. Do you mind if, as this PR was sent from your main branch and I cannot contribute to it, I close this PR and submit another PR with your commits and my suggestions on top? Then I can incorporate this new module in today's release
There was a problem hiding this comment.
Honestly, I am fine if you took the code and rebased into whatever location you need it to be :)
Just note that the issue which was blocking this module is still here:
I can not copy directories into container, if the source dir name and the dest dir name are not matching.
There is a test/example for that, but I placed an inverse condition for some reason.
I'll revert it now, so it would be clear why this is lagging for so long.
There was a problem hiding this comment.
Indeed, that limitation exists at the moment. My question now would be: why would the users of the firebase module like to provide a wrong dir name? I can see two types of users:
- developers of the firebase emulator, who would need different failure modes while developing it
- developers using the emulator, who would not need that particular failure mode.
I guess developers in group 1 would love to have multiple root dirs with different use cases. As a workaround I can think of a previous step that copies those dirs to a (t.TempDir() + "/firebase"), and then copy that temporary dir to the container. We can add now an issue to testcontainers to resolve the issue as soon as possible and remove that workaround right after it's released.
Of course, as a firebase noob, I'm probably missing other concerns you may have clear. Please let me know what you think about this 🙏
There was a problem hiding this comment.
The only reason why someone would have issues with directory name is when you have different setups for different tests.
In that case, the workaround seems to be a good solution.
There was a problem hiding this comment.
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
Just a quick pass as I'm busy today.
| Port uint16 `json:"port,omitempty"` | ||
| } `json:"tasks,omitempty"` | ||
| } | ||
| type partialFirebaseConfig struct { |
There was a problem hiding this comment.
nit: add a blank line between types, suspect it will fail linting without.
| ctx := context.Background() | ||
|
|
||
| firebaseContainer, err := firebase.Run(ctx, "ghcr.io/u-health/docker-firebase-emulator:13.29.2", | ||
| firebase.WithRoot(filepath.Join("testdata", "firebase")), |
There was a problem hiding this comment.
info: using literal "testdata/firebase" works fine on all platforms as even though windows uses \ it also accepts /.
| host := hostF.String() | ||
| if host != "0.0.0.0" { | ||
| return nil, fmt.Errorf("config specified %s emulator host on non public ip: %s", name, host) | ||
| } |
There was a problem hiding this comment.
question: this seems to limit host to 0.0.0.0, if so why have the field? that said I'm not sure this check is needed, do you have an example of why it is?
There was a problem hiding this comment.
If the emulator config (which we found in the user provided directory) uses host value other than "0.0.0.0", it will launch that emulator listening on docker internal IP. So no traffic will be able to enter the emulator, leaving the users very much confused.
| portF := emulator.FieldByName("Port") | ||
| websocketPortF := emulator.FieldByName("WebsocketPort") | ||
|
|
||
| if hostF != (reflect.Value{}) && !hostF.IsZero() { |
There was a problem hiding this comment.
suggestion: use IsValid to simplify test, more below
| if hostF != (reflect.Value{}) && !hostF.IsZero() { | |
| if hostF.isValid() && !hostF.IsZero() { |
| if emulator.Kind() != reflect.Struct { | ||
| continue | ||
| } | ||
| name := v.Type().Field(i).Name |
There was a problem hiding this comment.
suggestion: extract type lookup out of the loop as its invariant.
modules/firebase/firebase.go
Outdated
| bytes, err := io.ReadAll(cfg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("read firebase.json: %w", err) | ||
| } | ||
|
|
||
| var parsed partialFirebaseConfig | ||
| if err := json.Unmarshal(bytes, &parsed); err != nil { | ||
| return nil, fmt.Errorf("parse firebase.json: %w", err) | ||
| } |
There was a problem hiding this comment.
suggestion: use a stream reader to be more memory efficient, its a small benefit but good practice.
| if err != nil { | ||
| return nil, fmt.Errorf("gather ports: %w", err) | ||
| } | ||
| req.ExposedPorts = expectedExposedPorts |
There was a problem hiding this comment.
question: should we wait for all ports instead of the fragile log wait?
There was a problem hiding this comment.
I had the same idea, but now I don't remember why I reverted back to logs.
| // What would be a solution here? Previously I just added a check that the root must | ||
| // end in "/firebase"... I could do the same. | ||
| testcontainers.CleanupContainer(t, ctr) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
bug: this should fail, as I understand it this is down to how our copy works or doesn't. I have an unfinished PR in this area here.
I would change this to check for err as expected and mark the test as skip for now.
|
For reference, Google released an official image: firebase/firebase-tools#1644 (comment) |
|
Ah, cool. I'll adapt to that container once I have some free time. |
|
Fantastic! thanks for your time @xytis , we appreciate it! |
Co-authored-by: Manuel de la Peña <[email protected]> Co-authored-by: Steven Hartland <[email protected]>
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduces a new Firebase module for testcontainers-go, including container wrapper, configuration structures, customization options, integration tests, example code, test fixtures, and documentation. Enables launching Firebase emulator containers with configurable root paths, data directories, and caching. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Run as firebase.Run()
participant Config as Config Parser
participant TC as testcontainers
participant Container as Firebase<br/>Container
Test->>Run: Run(ctx, image, opts...)
Run->>Config: Read & parse firebase.json
Config-->>Run: emulatorsConfig
Run->>Run: Gather enabled ports via reflection
Run->>TC: Create GenericContainerRequest
Run->>TC: Start container with ports
TC->>Container: Initialize
Container-->>TC: Running container
TC-->>Run: testcontainers.Container
Run-->>Test: *Container wrapper
sequenceDiagram
participant Test
participant Container as Container
participant Firestore as Firestore Client
participant Emulator as Firebase<br/>Emulator
Test->>Container: ConnectionString(ctx, port)
Container->>Container: Host(ctx) + MappedPort(ctx)
Container-->>Test: "host:port"
Test->>Firestore: Initialize with URL
Test->>Emulator: Write document
Emulator-->>Test: Success
Test->>Emulator: Read document
Emulator-->>Test: Document data
Test->>Test: Assert value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/firebase/firebase.go`:
- Line 94: Replace the use of path.Join when building the filesystem path for
opening firebase.json: change the import to use the standard library filepath
package and use filepath.Join(userRoot, "firebase.json") in the os.Open call
(the variables/functions to update are the import list, the path.Join reference
in the line where cfg is assigned, and any other occurrences using path.Join
with host filesystem paths like userRoot). Ensure no other code relies on path
(URL) semantics for these filesystem paths.
In `@modules/firebase/go.mod`:
- Line 73: The project currently pulls an insecure indirect dependency
golang.org/x/oauth2 v0.24.0 (via cloud.google.com/go/firestore) which has
CVE-2025-22868; update the indirect module to v0.27.0 or later by running the
module update (e.g., use go get -u golang.org/x/[email protected]) and then run go
mod tidy to refresh modules so go.mod/go.sum reflect the upgraded
golang.org/x/oauth2 version referenced by cloud.google.com/go/firestore.
♻️ Duplicate comments (1)
docs/modules/firebase.md (1)
68-70: Documentation incomplete: Container Methods section is empty.The "Container Methods" section ends without listing any methods. Based on past review comments and the module implementation, the
ConnectionStringmethod should be documented here. Additionally, theWithDataoption mentioned in past reviews appears to be missing from the Container Options section.
🧹 Nitpick comments (8)
modules/firebase/ports.go (1)
10-22: Implementation is clean and follows testcontainers-go patterns.Error handling is well-structured with descriptive wrapping, and the blank lines after error returns improve readability.
Consider adding a brief godoc comment for this exported method to document its purpose and the expected
portNameparameter:📝 Suggested documentation
+// ConnectionString returns a connection string in the format "host:port" for the specified +// emulator port. The portName should match one of the ports exposed by the Firebase emulator +// configuration (e.g., "8080/tcp" for Firestore). func (c *Container) ConnectionString(ctx context.Context, portName nat.Port) (string, error) {modules/firebase/options.go (2)
7-19: Consider removing unusedOptiontype.The
Optiontype andoptionsstruct appear unused -OptionimplementsContainerCustomizerwith a NOOP but isn't used by the actual option functions (WithRoot,WithData,WithCache), which returntestcontainers.CustomizeRequestOptioninstead. If this scaffolding is for future use, consider adding a TODO comment explaining the intent.
41-51: Volume name prefix inconsistency: "firestore-cache" vs "firebase-cache".The volume caches Firebase binaries generally (at
/root/.cache/firebase), but the volume name uses"firestore-cache-"prefix. Consider renaming to"firebase-cache-"for consistency with the actual purpose.Proposed fix
func WithCache() testcontainers.CustomizeRequestOption { - volumeName := "firestore-cache-" + testcontainers.SessionID() + volumeName := "firebase-cache-" + testcontainers.SessionID() return testcontainers.WithMounts(testcontainers.ContainerMount{docs/modules/firebase.md (1)
13-15: Add language specifier to the fenced code block.The code block should specify a language for proper syntax highlighting and to satisfy markdown linting rules.
Proposed fix
-``` +```bash go get github.com/testcontainers/testcontainers-go/modules/firebase</details> </blockquote></details> <details> <summary>modules/firebase/examples_test.go (1)</summary><blockquote> `16-18`: **Consider using consistent Docker image versions across example and tests.** The example uses image version `13.29.2` while `firebase_test.go` uses version `15.3.0`. Consider aligning these versions for consistency and to ensure the example demonstrates the tested configuration. </blockquote></details> <details> <summary>modules/firebase/firebase_test.go (1)</summary><blockquote> `52-66`: **Clarify the expected outcome when this test is re-enabled.** The test is appropriately skipped pending an upstream fix. However, the assertion `require.NoError(t, err)` at line 65 contradicts the comment explaining that the test should demonstrate a failure case. When this test is eventually re-enabled, consider whether it should assert an error condition instead. </blockquote></details> <details> <summary>modules/firebase/firebase.go (2)</summary><blockquote> `38-60`: **Consider using `IsValid()` for reflect.Value checks.** The pattern `fieldValue != (reflect.Value{})` works but `IsValid()` is more idiomatic for checking if a reflect.Value was obtained from a valid field lookup. <details> <summary>♻️ Suggested refactor</summary> ```diff - enabledF := emulator.FieldByName("Enabled") - if enabledF != (reflect.Value{}) && !enabledF.Bool() { + if enabledF := emulator.FieldByName("Enabled"); enabledF.IsValid() && !enabledF.Bool() { continue } - hostF := emulator.FieldByName("Host") - portF := emulator.FieldByName("Port") - websocketPortF := emulator.FieldByName("WebsocketPort") - - if hostF != (reflect.Value{}) && !hostF.IsZero() { + if hostF := emulator.FieldByName("Host"); hostF.IsValid() && !hostF.IsZero() { host := hostF.String() if host != "0.0.0.0" { return nil, fmt.Errorf("config specified %s emulator host on non public ip: %s", name, host) } } - if portF != (reflect.Value{}) && !portF.IsZero() { + if portF := emulator.FieldByName("Port"); portF.IsValid() && !portF.IsZero() { port := fmt.Sprintf("%d/tcp", portF.Uint()) ports = append(ports, port) } - if websocketPortF != (reflect.Value{}) && !websocketPortF.IsZero() { + if websocketPortF := emulator.FieldByName("WebsocketPort"); websocketPortF.IsValid() && !websocketPortF.IsZero() { port := fmt.Sprintf("%d/tcp", websocketPortF.Uint()) ports = append(ports, port) }
109-109: ExposedPorts assignment overwrites any user-specified ports.If a customizer adds ports via
req.ExposedPorts, this line replaces them entirely. Consider appending instead to preserve user-configured ports.♻️ Suggested fix
- req.ExposedPorts = expectedExposedPorts + req.ExposedPorts = append(req.ExposedPorts, expectedExposedPorts...)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
modules/firebase/go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.vscode/.testcontainers-go.code-workspacedocs/modules/firebase.mdmkdocs.ymlmodules/firebase/Makefilemodules/firebase/config.gomodules/firebase/examples_test.gomodules/firebase/firebase.gomodules/firebase/firebase_test.gomodules/firebase/go.modmodules/firebase/options.gomodules/firebase/ports.gomodules/firebase/testdata/failure/.firebasercmodules/firebase/testdata/failure/firebase.jsonmodules/firebase/testdata/failure/firestore.indexes.jsonmodules/firebase/testdata/failure/firestore.rulesmodules/firebase/testdata/failure/storage.rulesmodules/firebase/testdata/firebase/.firebasercmodules/firebase/testdata/firebase/firebase.jsonmodules/firebase/testdata/firebase/firestore.indexes.jsonmodules/firebase/testdata/firebase/firestore.rulesmodules/firebase/testdata/firebase/storage.rules
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: natsoman
Repo: testcontainers/testcontainers-go PR: 3452
File: modules/cosmosdb/cosmosdb.go:11-14
Timestamp: 2025-11-05T11:57:09.224Z
Learning: In the testcontainers-go CosmosDB module (modules/azure/cosmosdb/), the defaultProtocol is intentionally set to "http" (not "https") to avoid requiring users to perform certificate setup steps (downloading and trusting the emulator's self-signed certificate). This design prioritizes ease of use for test containers over strict emulator fidelity.
📚 Learning: 2025-09-29T15:08:18.694Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3320
File: modules/artemis/artemis.go:98-103
Timestamp: 2025-09-29T15:08:18.694Z
Learning: In testcontainers-go, nat.Port is a type alias for string, so untyped string constants can be passed directly to functions expecting nat.Port (like wait.ForListeningPort) without explicit type conversion - the Go compiler handles the implicit conversion automatically.
Applied to files:
modules/firebase/ports.go
📚 Learning: 2025-11-05T11:57:09.224Z
Learnt from: natsoman
Repo: testcontainers/testcontainers-go PR: 3452
File: modules/cosmosdb/cosmosdb.go:11-14
Timestamp: 2025-11-05T11:57:09.224Z
Learning: In the testcontainers-go CosmosDB module (modules/azure/cosmosdb/), the defaultProtocol is intentionally set to "http" (not "https") to avoid requiring users to perform certificate setup steps (downloading and trusting the emulator's self-signed certificate). This design prioritizes ease of use for test containers over strict emulator fidelity.
Applied to files:
modules/firebase/ports.gomodules/firebase/go.mod
📚 Learning: 2025-09-18T08:24:27.479Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3254
File: .github/dependabot.yml:21-21
Timestamp: 2025-09-18T08:24:27.479Z
Learning: In the testcontainers-go repository, submodules like atlaslocal that are part of a parent module (e.g., mongodb) share the same go.mod file and should not have separate Dependabot entries. They are already monitored through the parent module's Dependabot configuration entry.
Applied to files:
docs/modules/firebase.mdmodules/firebase/go.mod.vscode/.testcontainers-go.code-workspace
📚 Learning: 2025-09-29T13:57:14.636Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3319
File: modules/arangodb/arangodb.go:46-57
Timestamp: 2025-09-29T13:57:14.636Z
Learning: In testcontainers-go ArangoDB module, the wait strategy combines port listening check with HTTP readiness check using wait.ForAll - both strategies are required and complementary, not redundant.
Applied to files:
modules/firebase/go.modmodules/firebase/firebase_test.go
🧬 Code graph analysis (4)
modules/firebase/ports.go (1)
modules/firebase/firebase.go (1)
Container(18-20)
modules/firebase/firebase.go (4)
options.go (1)
ContainerCustomizer(22-24)generic.go (2)
GenericContainerRequest(21-27)GenericContainer(52-98)container.go (2)
ContainerRequest(131-171)ContainerFile(110-115)wait/log.go (1)
ForLog(118-120)
modules/firebase/options.go (4)
options.go (3)
ContainerCustomizer(22-24)CustomizeRequestOption(28-28)WithFiles(524-529)generic.go (1)
GenericContainerRequest(21-27)container.go (1)
ContainerFile(110-115)mounts.go (1)
ContainerMount(133-140)
modules/firebase/examples_test.go (4)
modules/firebase/firebase.go (1)
Run(67-122)modules/firebase/options.go (2)
WithRoot(22-28)WithCache(42-51)cleanup.go (1)
TerminateContainer(97-108)log/logger.go (1)
Printf(47-49)
🪛 checkmake (0.2.2)
modules/firebase/Makefile
[warning] 3-3: Missing required phony target "all"
(minphony)
[warning] 3-3: Missing required phony target "clean"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
docs/modules/firebase.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.3.1)
modules/firebase/go.mod
[HIGH] 1-1: golang.org/x/oauth2 0.24.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 1-1: golang.org/x/oauth2 0.24.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
🔇 Additional comments (23)
modules/firebase/testdata/failure/.firebaserc (1)
1-5: LGTM!Standard Firebase project configuration file with appropriate test project name.
modules/firebase/testdata/failure/storage.rules (1)
1-8: LGTM!Permissive storage rules are appropriate for test fixtures. The
if truepattern provides unrestricted access needed for testing container functionality without security constraints.modules/firebase/testdata/firebase/firestore.indexes.json (1)
1-4: LGTM!Valid Firestore indexes configuration with empty arrays, appropriate for basic emulator test fixtures.
modules/firebase/testdata/firebase/.firebaserc (1)
1-5: LGTM!Standard Firebase project configuration file with appropriate test project name.
modules/firebase/Makefile (1)
1-5: LGTM!The Makefile follows the established pattern across all modules in the repository: include shared test infrastructure and delegate to module-specific targets.
modules/firebase/testdata/failure/firestore.rules (1)
1-8: LGTM!Valid Firestore security rules with a deny-all policy, appropriate for a failure test scenario.
modules/firebase/testdata/firebase/firestore.rules (1)
1-8: LGTM!Standard Firestore rules file with deny-all policy. Suitable as a test fixture for emulator configuration.
modules/firebase/testdata/firebase/storage.rules (1)
1-8: LGTM!Permissive storage rules appropriate for test fixtures where storage operations should succeed.
modules/firebase/testdata/firebase/firebase.json (1)
1-39: Configuration looks good overall.The emulator configuration correctly binds all services to
0.0.0.0for container accessibility, and uses standard Firebase emulator ports. The inline comment about UI limitations in containers is helpful. ThesingleProjectModesetting is correctly placed inside theemulatorsobject per Firebase configuration schema..vscode/.testcontainers-go.code-workspace (1)
92-95: LGTM!The Firebase module entry is correctly added in alphabetical order and follows the established naming convention (
module / firebase) consistent with other modules in the workspace.modules/firebase/config.go (1)
1-87: LGTM!The configuration structs are well-structured and appropriately model the Firebase emulator configuration. The types are correctly unexported for internal use, port fields use
uint16, and JSON tags are properly applied withomitempty.modules/firebase/go.mod (1)
1-12: LGTM on module structure.The module declaration, Go version, and direct dependencies are appropriate for the Firebase module. The replace directive correctly points to the parent module for local development.
modules/firebase/testdata/failure/firestore.indexes.json (1)
1-4: LGTM!Valid Firestore indexes configuration file for test fixtures.
modules/firebase/options.go (1)
32-37: The nil map concern does not apply to this code in the Firebase context.Firebase's
Run()function always initializesreq.Envto an empty map at line 72 before applying anyCustomizeRequestOptionfunctions, soWithDatawill never encounter a nil map. The code is correct and does not need the proposed fix.Likely an incorrect or invalid review comment.
mkdocs.yml (1)
95-95: LGTM!The Firebase module documentation entry is correctly added in alphabetical order within the Modules navigation section.
modules/firebase/testdata/failure/firebase.json (1)
1-39: LGTM!The Firebase emulator configuration for the failure test scenario is well-structured. The inline comment explaining UI limitations in containerized environments is helpful context.
modules/firebase/examples_test.go (1)
13-40: LGTM!The example correctly demonstrates the Firebase module usage pattern:
- Proper cleanup with deferred
TerminateContainerbefore error check (handles nil container gracefully)- Uses
log.Printfinstead oflog.Fatalto ensure defers run- Cross-platform path handling with
filepath.Joinmodules/firebase/firebase_test.go (2)
17-50: LGTM!This integration test is well-structured:
- Proper context with deadline and cancellation
- Correct cleanup pattern with
CleanupContainerbefore error assertion- Uses
t.Setenvfor environment variables (auto-restored after test)- Uses modern
anytype- Verifies end-to-end Firestore write/read cycle
68-75: LGTM!Good negative test case that verifies
RunreturnsErrRootNotProvidedwhen the required root option is missing. The error assertion withrequire.ErrorIsis the correct approach.modules/firebase/firebase.go (4)
1-15: Imports look appropriate for the module's functionality.The package structure and imports align with other testcontainers-go modules. One note: verify that
path(line 9) vspath/filepathis intentional—see related comment on line 94.
17-25: LGTM!The
Containerwrapper and sentinel error follow standard testcontainers-go module patterns.
66-90: LGTM!The request setup follows testcontainers-go conventions. The log-based wait strategy is appropriate for detecting emulator readiness, and the root path validation ensures required configuration is present.
111-119: LGTM!The error handling correctly returns the container even on failure, allowing callers to perform cleanup. This follows the testcontainers-go pattern for handling partial creation failures.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| // Parse expected emulators from the root: | ||
| userRoot := req.Files[rootPathIdx].HostFilePath | ||
| cfg, err := os.Open(path.Join(userRoot, "firebase.json")) |
There was a problem hiding this comment.
Use filepath.Join instead of path.Join for host filesystem paths.
path.Join is for URL-style forward-slash paths, while filepath.Join handles OS-specific path separators. On Windows, path.Join won't correctly handle backslashes in userRoot.
🔧 Proposed fix
Update the import:
- "path"
+ "path/filepath"And the usage:
- cfg, err := os.Open(path.Join(userRoot, "firebase.json"))
+ cfg, err := os.Open(filepath.Join(userRoot, "firebase.json"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg, err := os.Open(path.Join(userRoot, "firebase.json")) | |
| cfg, err := os.Open(filepath.Join(userRoot, "firebase.json")) |
🤖 Prompt for AI Agents
In `@modules/firebase/firebase.go` at line 94, Replace the use of path.Join when
building the filesystem path for opening firebase.json: change the import to use
the standard library filepath package and use filepath.Join(userRoot,
"firebase.json") in the os.Open call (the variables/functions to update are the
import list, the path.Join reference in the line where cfg is assigned, and any
other occurrences using path.Join with host filesystem paths like userRoot).
Ensure no other code relies on path (URL) semantics for these filesystem paths.
| go.opentelemetry.io/proto/otlp v1.0.0 // indirect | ||
| golang.org/x/crypto v0.45.0 // indirect | ||
| golang.org/x/net v0.47.0 // indirect | ||
| golang.org/x/oauth2 v0.24.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat modules/firebase/go.modRepository: testcontainers/testcontainers-go
Length of output: 4101
🌐 Web query:
golang.org/x/oauth2 GO-2025-3488 vulnerability
💡 Result:
Summary: GO-2025-3488 (CVE-2025-22868) is a high-severity bug in golang.org/x/oauth2 (package golang.org/x/oauth2/jws) where a specially crafted malformed token can cause unexpected memory consumption during parsing (potential DoS). Affects all versions before v0.27.0; fixed in v0.27.0. Recommended action: upgrade to golang.org/x/oauth2 v0.27.0 or later. [1][2][3][4]
References:
- pkg.go.dev vulnerability entry (GO-2025-3488 / CVE-2025-22868). [1]
- OSV / API entry for GO-2025-3488. [2]
- GitHub Advisory (GHSA-6v2p-p543-phr9) / advisory details. [3]
- Debian / security tracker entry (package fixes and status). [4]
🌐 Web query:
golang.org/x/oauth2 GHSA-6v2p-p543-phr9 security advisory
💡 Result:
Short summary:
- GHSA-6v2p-p543-phr9 refers to golang.org/x/oauth2 (alias CVE-2025-22868 / GO-2025-3488). [1][2]
- Affected versions: all releases before v0.27.0. Patched in v0.27.0. [1][2]
- Impact: malformed tokens can trigger unexpected memory consumption during parsing (DoS risk). Severity: High (CVSS 7.5). [1][3]
- Mitigation: upgrade to v0.27.0 or later (e.g. run: go get golang.org/x/[email protected]). [2][3]
Sources: GitHub Advisory (GHSA-6v2p-p543-phr9). [1] OSV / pkg.go.dev (GO-2025-3488 / CVE-2025-22868). [2] Vendor/analysis summaries (Wiz/NVD references). [3]
🌐 Web query:
golang.org/x/oauth2 v0.24.0 latest version 2025
💡 Result:
v0.24.0 is not the latest. The module has been updated; the newest tagged release is v0.30.0. [1][2]
Sources:
- go.googlesource.com/oauth2 — tags listing showing v0.30.0. [1]
- go.googlesource.com/oauth2 — refs/tags/v0.24.0. [2]
Security vulnerability in indirect dependency golang.org/x/oauth2 v0.24.0.
Static analysis detected HIGH severity vulnerability GO-2025-3488 (CVE-2025-22868 / GHSA-6v2p-p543-phr9) in golang.org/x/oauth2 v0.24.0: malformed tokens can cause unexpected memory consumption during parsing, leading to DoS risk.
This is an indirect dependency pulled in via cloud.google.com/go/firestore. Upgrade to v0.27.0 or later by running go get -u golang.org/x/[email protected] followed by go mod tidy.
🤖 Prompt for AI Agents
In `@modules/firebase/go.mod` at line 73, The project currently pulls an insecure
indirect dependency golang.org/x/oauth2 v0.24.0 (via
cloud.google.com/go/firestore) which has CVE-2025-22868; update the indirect
module to v0.27.0 or later by running the module update (e.g., use go get -u
golang.org/x/[email protected]) and then run go mod tidy to refresh modules so
go.mod/go.sum reflect the upgraded golang.org/x/oauth2 version referenced by
cloud.google.com/go/firestore.
3 participants
What does this PR do?
This module allows running firebase emulator as a testcontainer.
It does currently depend on a forked docker image, residing under my ownership. There is no official firebase emulator docker image to my knowledge.
Why is it important?
We use firebase, thought someone else might be using firebase too.