feat(martin-server): add support for basic CORS configuration

[NINNiT]

As mentioned in #931, I've tried to come up with a basic implementation/spec which allows configuration of CORS..

Before:
Static CORS configuration in server.rs.

let cors_middleware = Cors::default()
         .allow_any_origin()
         .allowed_methods(vec!["GET"]);

This translates to the following logic:

• If an incoming request has an Origin Header, the value of this Headers is used to set Access-Control-Allow-Origin (not *)
• If an incoming request has no Origin Header, Access-Control-Allow-Origin is not set, however vary headers are still being sent

After:

• Same default behavior as before (CORS enabled, all origins allowed, only GET methods allowed). There should be no breaking changes for existing deployments.
• Allows configuration of the following properties:

# CORS Configuration
# Can also be disabled via `cors: false`
cors:
  # Sets the `Access-Control-Allow-Origin` header [default: *]
  # '*' will use the requests `ORIGIN` header
  origin:
    - https://example.org
  # Sets `Access-Control-Max-Age` Header. [default: null]
  # null means not setting the header for preflight requests
  max_age: 3600

• Conditionally wraps the actix app in the CORS middleware, if the latter is enabled

   app.wrap(Condition::new(
            cors_middleware.is_some(),
            cors_middleware.unwrap_or_default(),
        ))

• Adds tests for CORS

• Config is nested under SrvConfig . I think this is fitting, as it's not a source.

Let me know if you have any suggestions, changes that should be applied or if you'd like to take this over. 🦀
@CommanderStorm im gonna ping you here, hope that's okay

[Copilot]

Pull Request Overview

This PR introduces configurable CORS support to the Martin server while preserving existing default behavior. Key changes include:

• Adding a new CorsConfig and its make_cors_middleware method.
• Updating server.rs to conditionally apply CORS middleware based on configuration.
• Incorporating new tests and updating configuration documentation.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
martin/tests/cors_test.rs	Added tests to validate the new CORS configuration behavior.
martin/src/srv/server.rs	Updated middleware setup to wrap the app conditionally with the CORS middleware.
martin/src/srv/mod.rs	Imported the new CORS module.
martin/src/srv/cors.rs	Introduced CorsConfig struct with middleware creation logic for CORS handling.
martin/src/srv/config.rs	Integrated CorsConfig into SrvConfig and added a YAML parsing test for CORS settings.
docs/src/config-file.md	Updated the configuration documentation to include CORS setup examples.

Comments suppressed due to low confidence (2)

docs/src/config-file.md:53

• The configuration documentation uses 'allowed_origins' while the code uses 'origin'. Consider aligning these names for consistency.

allowed_origins:

martin/src/srv/cors.rs:8

• Consider renaming the field 'origin' to 'allowed_origins' to match the configuration documentation and improve clarity.

pub origin: Vec<String>,

[Copilot]

Pull Request Overview

The PR adds configurable CORS support to the martin-server, ensuring backward compatibility while allowing deployment-specific CORS configuration via the server config and YAML file. It also includes updated tests and documentation for the new CORS feature.

• Introduces a new CorsConfig with middleware generation in its own module.
• Updates the server initialization to conditionally wrap the app with CORS middleware based on configuration.
• Adds relevant tests and documentation for the configurable CORS settings.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
martin/tests/cors_test.rs	Adds tests to verify CORS behavior under various configurations.
martin/src/srv/server.rs	Updates server creation to use the new configurable CORS middleware.
martin/src/srv/mod.rs	Registers the new CORS module.
martin/src/srv/cors.rs	Implements CorsConfig and middleware generation logic.
martin/src/srv/config.rs	Updates SrvConfig to include CorsConfig and tests YAML parsing.
docs/src/config-file.md	Enhances documentation with new CORS configuration options.

Comments suppressed due to low confidence (1)

martin/src/srv/server.rs:187

• [nitpick] Consider caching the result of cors_middleware.unwrap_or_default() in a variable to avoid calling it twice, which can improve readability and reduce the chance of future errors if the unwrap logic evolves.

app.wrap(Condition::new(cors_middleware.is_some(), cors_middleware.unwrap_or_default(),))

[nyurik]

need to document what does true means, and what happens if not set at all (default). Note that default here and in sub-items is a bit confusing, i.e. does cors: {} equal to cors: ~ ?

[NINNiT]

good point - i'll expand on that and double check what happens. might even add some tests
never seen ~ or {} in the wild though.

[NINNiT]

Right now, both cors: {} and cors: ~ seem to enable CORS using our defaults.

[NINNiT]

As it stands right now, it'll always fall back to the defaults.

• no cors option at all -> Enable using defaults
• cors: true -> Enable using defaults
• only configuring a subset and omitting certain properties-> use defaults for properties that are not explicitly set.

this...

cors:
  max_age: 3600

would translate to this:

cors:
  origin: ["*"]
  max_age: 3600

The only way to disable CORS right now, is to explicitly set cors: false.

I guess cors: ~ should also disable it? What would you prefer

[nyurik]

No, null is the same as skipped, so it should be the default

[NINNiT]

I'm wondering if we actually want to fall back to defaults when CORS is explicitly configured.
For example, when only a subset of properties is specified (e.g., just max_age), should we implicitly apply defaults to the omitted properties?

This:

cors:
  max_age: 3600

would currently be interpreted as:

cors:
  origin: ["*"] # default value
  max_age: 3600

Is that the intended behavior, or should partial configuration require more explicit values to avoid unintended/hidden defaults?

[NINNiT]

@CommanderStorm
Some of your test changes aren't compiling on my end (stable toolchain, v1.87.0)

error[E0369]: binary operation `==` cannot be applied to type `std::result::Result<std::option::Option<Cors>, utils::error::MartinError>`
   --> martin/src/srv/cors.rs:121:9
    |
121 |         assert_eq!(config.make_cors_middleware(), Ok(None));
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         std::result::Result<std::option::Option<Cors>, utils::error::MartinError>
    |         std::result::Result<std::option::Option<Cors>, utils::error::MartinError>
    
    note: an implementation of `PartialEq` might be missing for `utils::error::MartinError`
   --> martin/src/utils/error.rs:30:1
    |
30  | pub enum MartinError {
    | ^^^^^^^^^^^^^^^^^^^^ must implement `PartialEq`

Looks like we'll have to implement PartialEq for MartinError
or assert Ok and Option separately (the latter after unwrapping)

[CommanderStorm]

Good Job.
I have included a few nits and will finish up the rest when I have more stable Internet.

Sorry for the long march this feature has been and thanks for completing it.
We really appreciate the effort ♥️

[CommanderStorm]

I need to check if logs are printed.
Might need to move this up a bit.

[NINNiT]

If we are through with all the changes, I'd like to test this one last time in the real world.
Just to be sure, before this makes it into the release 😆

[CommanderStorm]

I need to check if logs are printed.
Might need to move this up a bit.

I also need to check this part ^^

[Copilot]

Pull Request Overview

This pull request introduces configurable CORS support for the Martin server while preserving default behavior for existing deployments. Key changes include:

• Adding a new CORS middleware module (martin/src/srv/cors.rs) that builds the actix_cors middleware based on configuration.
• Updating the server initialization to validate and conditionally apply the CORS middleware.
• Expanding tests and documentation to cover various CORS configurations.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
martin/tests/cors_test.rs	New tests added to validate different CORS configuration scenarios.
martin/src/utils/error.rs	Introduces a new CorsError variant for error handling in CORS logic.
martin/src/srv/server.rs	Updates server setup to integrate and validate CORS middleware.
martin/src/srv/mod.rs	Exposes the new cors module to the rest of the application.
martin/src/srv/cors.rs	New module implementing the CORS configuration and middleware creation.
martin/src/srv/config.rs	Extends server configuration to include CORS settings.
docs/src/config-file.md	Documents the new CORS configuration options.
.dockerignore	Updates ignore patterns by excluding .aider* files.

[nyurik]

Thank you!!!