Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an AI Agent Service Token mechanism and administrative roles to the LMeterX platform, enabling programmatic access for tools like OpenClaw and Cursor. Key changes include the addition of the lmeterx-web-loadtest skill, server-side path whitelisting for service tokens, and permission bypasses for admin users. Security hardening was applied to log access and LDAP authentication, alongside various frontend improvements for task management and result comparison. Feedback focused on security risks associated with hardcoded default tokens and disabled SSL verification, as well as inconsistencies in default configuration values.
.openclaw/env.example
Outdated
| LMETERX_BASE_URL= | ||
| LMETERX_AUTH_TOKEN= | ||
| LMETERX_BASE_URL=http://localhost:8080 | ||
| LMETERX_AUTH_TOKEN=localhost_lmeterx |
There was a problem hiding this comment.
| export LMETERX_AUTH_TOKEN="lmeterx" | ||
| python "${SKILL_DIR}/scripts/run.py" --url "<web URL>" |
There was a problem hiding this comment.
The example command hardcodes the LMETERX_AUTH_TOKEN to "lmeterx". This is a security risk as it encourages the use of a weak, guessable token. The documentation should use a placeholder like "<your-token-here>" to remind users to use a secure, unique token.
| export LMETERX_AUTH_TOKEN="lmeterx" | |
| python "${SKILL_DIR}/scripts/run.py" --url "<web URL>" | |
| export LMETERX_AUTH_TOKEN="<your-token-here>" | |
| python "${SKILL_DIR}/scripts/run.py" --url "<web URL>" |
| ) | ||
|
|
||
| # Prioritize getting Service Token from environment variables; if not configured, use the built-in default value "localhost_lmeterx". | ||
| LMETERX_AUTH_TOKEN: str = os.getenv("LMETERX_AUTH_TOKEN") or "localhost_lmeterx" |
There was a problem hiding this comment.
Hardcoding a default service token ("localhost_lmeterx") is a security risk. This weak token could be inadvertently used in a production setting. It's safer to require the LMETERX_AUTH_TOKEN to be explicitly set as an environment variable and not provide a default value in the code. If no token is provided, the application should handle it gracefully (e.g., by having an empty string as default).
| LMETERX_AUTH_TOKEN: str = os.getenv("LMETERX_AUTH_TOKEN") or "localhost_lmeterx" | |
| LMETERX_AUTH_TOKEN: str = os.getenv("LMETERX_AUTH_TOKEN") or "" |
| _validate_url(args.url) | ||
| _validate_concurrency(args.concurrent_users) | ||
|
|
||
| with httpx.Client(verify=False) as client: |
There was a problem hiding this comment.
Using httpx.Client(verify=False) disables SSL/TLS certificate validation, which is a significant security risk. This makes the connection vulnerable to man-in-the-middle attacks. While this might be acceptable for local development against a self-signed certificate, it should not be the default for all environments. Consider making SSL verification configurable, for example, via an environment variable, and defaulting to True for security.
| with httpx.Client(verify=False) as client: | |
| with httpx.Client(verify=(os.getenv("LMETERX_SSL_VERIFY", "true").lower() not in ("false", "0", "no"))) as client: |
| LMETERX_BASE_URL: str = os.getenv("LMETERX_BASE_URL", "https://localhost:8080").rstrip( | ||
| "/" | ||
| ) |
There was a problem hiding this comment.
The default value for LMETERX_BASE_URL is https://localhost:8080, but in .openclaw/env.example and README.md it is http://localhost:8080. This inconsistency can lead to confusion and connection errors. Please make them consistent. Using http for local development is common.
| LMETERX_BASE_URL: str = os.getenv("LMETERX_BASE_URL", "https://localhost:8080").rstrip( | |
| "/" | |
| ) | |
| LMETERX_BASE_URL: str = os.getenv("LMETERX_BASE_URL", "http://localhost:8080").rstrip( | |
| "/" | |
| ) |
support api token and admin account and add lmeterx-web-loadtest skill