chore(plugins): align rate-limiter config with gateway convention

[gandhipratik203]

Summary

Align the rate-limiter plugin's redis_url in plugins/config.yaml with the gateway's existing convention of sourcing Redis URLs from the REDIS_URL env via the plugin loader's Jinja substitution. Operators set REDIS_URL once and the rate-limiter picks it up — no more hand-editing the yaml or templating it in deployment tooling.

Companion to #4581.

Changes

• plugins/config.yaml: redis_url: "redis://redis:6379/0" → redis_url: '{{ env.REDIS_URL | default("redis://redis:6379/0") }}' plus a short comment about the env-source. Outer single / inner double quotes survive yaml.safe_dump round-trips cleanly.
• tests/integration/test_rate_limiter_redis_url_from_yaml.py: new test — env-var substitution flows through the plugin loader cleanly and the resolved URL reaches a live Redis (PING → PONG). Skips when Redis isn't reachable.

Test plan

• pytest tests/integration/test_rate_limiter_redis_url_from_yaml.py green locally with Redis running
• Skips cleanly without Redis
• CI green

The schema-regression test and redis_fallback cleanup originally bundled here are now tracked as #4603.

[msureshkumar88]

Me review your PR! Changes GOOD idea but have few problems that MUST fix before merge.

🔴 CRITICAL Issues (Must Fix Now!)

1. Bare Exception Swallow All Errors 🚨

• Location: test_plugins_config_yaml_validation.py lines 116-120
• Problem: Code catch ALL exceptions, even security failures!
• Attack: Bad plugin could bypass rate limiting if fail to load
• Fix: Only catch connection errors, let security failures FAIL test

2. No URL Validation 🚨

• Location: test_plugins_config_yaml_validation.py lines 54-61
• Problem: No check if redis_url is safe! Could be SSRF attack vector
• Attack: REDIS_URL pointing to internal-admin service could steal data
• Fix: Validate URL scheme is redis, check hostname not internal, mask credentials

3. Breaking Change: redis_fallback Removal Not Justified ⚠️

• You remove redis_fallback: true but:
  • Still in docs (plugin-bindings-api.md line 345)
  • Still in Helm charts (values-pgo.yaml line 101)
  • Still in tests (test_rate_limiter.py line 1267)
  • Existing test validate this behavior!
• Impact: If Redis down, what happen? Fail open (no rate limit) or fail closed (deny all)?
• Fix: Show evidence key is unrecognized OR revert change OR update ALL docs/tests

🟡 Should Fix (Recommended)

4. Performance: Use async Redis client instead of blocking sync call
5. Testing Gap: Add negative tests (bad URLs, missing env vars, malformed Jinja)
6. Logging Gap: Add audit trail when config changes

Read full report for:

• All issues with code examples
• Alternative approaches you could use
• Risk assessment matrix
• Specific code fixes for each issue

✅ What Need Before Merge

MUST FIX (Blocking):

1. Fix bare exception handling
2. Add URL validation
3. Justify redis_fallback removal OR revert (breaking change)
4. Update ALL documentation consistently

SHOULD FIX:
5. Use async Redis client
6. Add negative test cases
7. Add audit logging

🎯 Verdict

⚠️ REQUEST CHANGES - Critical issues found

Estimated effort: 4-6 hours to fix all critical issues

[msureshkumar88]

As mentioned in comments

[gandhipratik203]

Thanks for the review. Scope-narrowing this PR to just the redis_url Jinja substitution: the schema-validation regression test and redis_fallback removal that triggered most of these comments have moved to #4603, which catalogues the broader cleanup (docs, helm chart, perf configs, unit fixtures, and a misleadingly-named integration test). Starting-point branch: chore/rate-limiter-redis-fallback-cleanup-wip.

That moots 1 and 3 of your review (both lived in the code that's been moved out).

2 (SSRF on REDIS_URL): the test sets the env var itself via monkeypatch.setenv to localhost — there is no untrusted input crossing a trust boundary, so SSRF doesn't apply. Validating a value the test just wrote would be testing our own hardcoded string.

4–6: not applicable in the narrowed scope. A one-shot Redis PING doesn't need an async client; a positive-path test that env-var substitution flows through the loader cleanly doesn't need adversarial inputs; nothing mutates config at runtime here so there's no audit surface.

[msureshkumar88]

Good direction — the Jinja | default(...) pattern matches what the rest of config.yaml already does for IBM_WATSON_*, and the outer-single/inner-double quoting to survive yaml.safe_dump round-trips is a nice touch. A couple of things worth addressing before this lands:

Blocking

Relative path in the integration test

cfg = ConfigLoader.load_config("plugins/config.yaml")   # test line 56

This resolves from cwd, so the test silently breaks whenever pytest is invoked from any directory other than the repo root (e.g. cd tests && pytest). Suggest anchoring to the file's own location:

import pathlib
_REPO_ROOT = pathlib.Path(__file__).resolve().parents[3]
cfg = ConfigLoader.load_config(str(_REPO_ROOT / "plugins" / "config.yaml"))

Security Scan CI failure

25/28 checks pass but the Security Scan job is failing. Worth checking whether Bandit is flagging something in the new test file (e.g. the bare except Exception in _redis_reachable, or the socket usage) and either suppressing with a # nosec + justification comment or adjusting the code. Happy to take a look at the job log if you can share it.

Suggestions

No test for the default fallback path

The test always injects REDIS_URL via monkeypatch.setenv, so the | default("redis://redis:6379/0") branch is never exercised. A small unit test (no live Redis needed) would pin that:

def test_rate_limiter_redis_url_uses_default_when_env_unset(monkeypatch, tmp_path):
    monkeypatch.delenv("REDIS_URL", raising=False)
    cfg = ConfigLoader.load_config("plugins/config.yaml")
    rl = next(p for p in cfg.plugins if p.name == "RateLimiterPlugin")
    assert rl.config.get("redis_url") == "redis://redis:6379/0"

(Same relative-path caveat applies here.)

Minor

The comment in config.yaml mentions WASM sandboxes — a bit surprising in a Redis URL context, since WASM plugins typically can't open TCP sockets. Trimming to just "separate container" would avoid confusion:

# Sourced from the gateway's REDIS_URL env; when the plugin runs in a
# separate container, ensure that host is reachable from within it.

---

One pre-existing issue worth tracking separately (not blocking this PR): SandboxedEnvironment in config.py:80 is constructed with autoescape=True, which HTML-encodes &, <, >, ". That's correct for HTML templates but wrong for YAML — if REDIS_URL ever includes a percent-encoded password or similar, the rendered value would be silently corrupted. Probably worth a follow-up issue since it affects all Jinja-templated values in the config, not just this one.

[gandhipratik203]

Thanks — this was a great review. Addressed all four actionable items in aa5ded9; each backported to the WIP branch for #4603 in ee9436e so the path-anchor fix carries forward when that work picks up.

Relative path — anchored via pathlib.Path(__file__).resolve().parents[2]. Verified the test passes both from repo root and from tests/.

Security Scan failure — the job log says The job was not acquired by Runner of type hosted even after multiple attempts (queued 1h30m, abandoned). Infra, not code. The push to aa5ded9 re-triggered CI; the new Security Scan run is in progress.

Default fallback test — added test_rate_limiter_redis_url_uses_default_when_env_unset exactly as suggested. Runs without Redis.

WASM comment — trimmed to the separate-container case as suggested. You're right that WASM plugins can't open TCP sockets, so REDIS_URL doesn't apply to that path.

autoescape pre-existing bug — filed as #4605. Sharp catch; agree it's broader than this PR (affects every Jinja-templated value, not just redis_url) and warrants its own fix.