Skip to content

Commit 6bd0c89

Browse files
ammarioclaude
ammario
and
claude
committed
fix: Address PR review comments
- Fix V8 platform lifetime issue by storing SharedRef in OnceLock - Update JS and prog engines to use deny_message instead of message - Support shorthand {deny_message: "..."} syntax (implies allow: false) - Update README to clarify JS is fastest mode - Only return messages when denying, not when allowing Breaking change: JS expressions now use deny_message instead of message for custom denial messages, matching the prog engine API. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent 49e206e commit 6bd0c89

File tree

3 files changed

+90
-66
lines changed

3 files changed

+90
-66
lines changed

‎README.md‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -252,12 +252,13 @@ Examples:
252252
true // Allow
253253
false // Deny
254254

255-
// Object with message (parentheses needed when used as expression)
256-
({allow: false, message: "Blocked by policy"})
257-
({allow: true, message: "Allowed with warning"})
255+
// Object with deny_message (parentheses needed when used as expression)
256+
({allow: false, deny_message: "Blocked by policy"})
257+
// Shorthand: if only deny_message is provided, request is denied
258+
({deny_message: "Blocked by policy"})
258259

259260
// Conditional with message
260-
r.method === 'POST' ? {allow: false, message: 'POST not allowed'} : true
261+
r.method === 'POST' ? {deny_message: 'POST not allowed'} : true
261262
```
262263

263264
**JavaScript evaluation rules:**
@@ -269,10 +270,11 @@ r.method === 'POST' ? {allow: false, message: 'POST not allowed'} : true
269270

270271
**Performance considerations:**
271272

272-
- V8 engine provides fast JavaScript execution
273-
- Fresh isolate creation per request ensures thread safety but adds some overhead
274-
- JavaScript evaluation is generally faster than shell script execution (--sh)
275-
- Persistent program mode (--prog) can be comparable to JavaScript for compiled languages
273+
- JavaScript mode is designed to be the fastest evaluation mode
274+
- V8 engine provides fast JavaScript execution with minimal overhead
275+
- Fresh isolate creation per request ensures thread safety
276+
- JavaScript evaluation is significantly faster than shell script execution (--sh)
277+
- Persistent program mode (--prog) can approach JavaScript performance for compiled languages, and future versions may support parallel instances and stateful caching
276278

277279
> [!NOTE]
278280
> The evaluation flags `--js`, `--js-file`, `--sh`, and `--prog` are mutually exclusive. Only one evaluation method can be used at a time.
@@ -302,7 +304,7 @@ fi
302304

303305
### Persistent Program Mode (--prog)
304306

305-
The `--prog` flag starts a single persistent process that receives JSON-formatted requests on stdin (one per line) and outputs decisions line-by-line. This approach eliminates process spawn overhead by keeping the evaluator in memory, making it suitable for production use.
307+
The `--prog` flag starts a single persistent process that receives JSON-formatted requests on stdin (one per line) and outputs decisions line-by-line. This approach eliminates process spawn overhead by keeping the evaluator in memory, making it suitable for production use. The API is designed to be equivalent to the JavaScript engine, supporting the same response formats.
306308

307309
```bash
308310
# Use a persistent program (path to executable)
@@ -335,8 +337,9 @@ for line in sys.stdin:
335337
336338
- **Output**: One response per line, either:
337339
- Simple: `true` or `false` (matching JS boolean semantics)
338-
- JSON: `{"allow": true/false, "message": "optional context"}`
339-
- Any other output is treated as deny with the output as the message
340+
- JSON: `{"allow": true/false, "deny_message": "optional message for denials"}`
341+
- Shorthand: `{"deny_message": "reason"}` (implies allow: false)
342+
- Any other output is treated as deny with the output as the deny_message
340343
341344
> [!NOTE]
342345
> Make sure to flush stdout after each response in your script to ensure real-time processing!

‎src/rules/prog.rs‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -168,23 +168,29 @@ impl ProgRuleEngine {
168168
if let Ok(json_response) =
169169
serde_json::from_str::<serde_json::Value>(response)
170170
{
171-
let allowed = json_response
172-
.get("allow")
173-
.and_then(|v| v.as_bool())
174-
.unwrap_or(false);
175-
176-
let message = json_response
177-
.get("message")
171+
// Check for deny_message first
172+
let deny_message = json_response
173+
.get("deny_message")
178174
.and_then(|v| v.as_str())
179175
.map(String::from);
180176

177+
// Get allow value - if not present but deny_message exists, default to false
178+
let allowed = if let Some(allow_val) = json_response.get("allow") {
179+
allow_val.as_bool().unwrap_or(false)
180+
} else if deny_message.is_some() {
181+
// Shorthand: if only deny_message is present, it implies allow: false
182+
false
183+
} else {
184+
false
185+
};
186+
181187
if allowed {
182188
debug!("ALLOW: {} {} (script allowed via JSON)", method, url);
189+
(allowed, None) // No message when allowing
183190
} else {
184191
debug!("DENY: {} {} (script denied via JSON)", method, url);
192+
(allowed, deny_message)
185193
}
186-
187-
(allowed, message)
188194
} else {
189195
// Not JSON, treat the output as a deny message
190196
debug!("DENY: {} {} (script returned: {})", method, url, response);
@@ -347,13 +353,13 @@ for line in sys.stdin:
347353
try:
348354
request = json.loads(line)
349355
if 'blocked' in request.get('host', ''):
350-
response = {'allow': False, 'message': f"Host {request['host']} is blocked"}
356+
response = {'allow': False, 'deny_message': f"Host {request['host']} is blocked"}
351357
else:
352358
response = {'allow': True}
353359
print(json.dumps(response))
354360
sys.stdout.flush()
355361
except Exception as e:
356-
print(json.dumps({'allow': False, 'message': str(e)}))
362+
print(json.dumps({'allow': False, 'deny_message': str(e)}))
357363
sys.stdout.flush()
358364
"#;
359365
script_file.write_all(script.as_bytes()).unwrap();
@@ -453,18 +459,18 @@ for line in sys.stdin:
453459
454460
# First request: allow
455461
if counter == 1:
456-
response = {"allow": True, "message": f"Request {counter} allowed"}
462+
response = {"allow": True}
457463
print(json.dumps(response))
458464
sys.stdout.flush()
459-
# Second request: allow then exit
465+
# Second request: deny then exit
460466
elif counter == 2:
461-
response = {"allow": False, "message": f"Request {counter} denied, exiting"}
467+
response = {"allow": False, "deny_message": f"Request {counter} denied, exiting"}
462468
print(json.dumps(response))
463469
sys.stdout.flush()
464470
sys.exit(0) # Exit after second request
465471
else:
466472
# This should never be reached in the first process
467-
response = {"allow": True, "message": f"Unexpected request {counter}"}
473+
response = {"allow": True}
468474
print(json.dumps(response))
469475
sys.stdout.flush()
470476
"#;
@@ -488,12 +494,7 @@ for line in sys.stdin:
488494
.evaluate(Method::GET, "https://example.com/1", "127.0.0.1")
489495
.await;
490496
assert!(matches!(result.action, Action::Allow));
491-
assert!(
492-
result
493-
.context
494-
.as_ref()
495-
.is_some_and(|c| c.contains("Request 1 allowed"))
496-
);
497+
assert_eq!(result.context, None); // No message when allowing
497498

498499
// Second request - should be denied, then process exits
499500
let result = engine
@@ -515,12 +516,7 @@ for line in sys.stdin:
515516
.evaluate(Method::GET, "https://example.com/3", "127.0.0.1")
516517
.await;
517518
assert!(matches!(result.action, Action::Allow));
518-
assert!(
519-
result
520-
.context
521-
.as_ref()
522-
.is_some_and(|c| c.contains("Request 1 allowed"))
523-
);
519+
assert_eq!(result.context, None); // No message when allowing
524520

525521
// Fourth request - should be second request in restarted process
526522
let result = engine

‎src/rules/v8_js.rs‎

Lines changed: 53 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,15 @@ pub struct V8JsRuleEngine {
1414

1515
impl V8JsRuleEngine {
1616
pub fn new(js_code: String) -> Result<Self, Box<dyn std::error::Error>> {
17-
// Initialize V8 platform once
18-
static V8_INIT: std::sync::Once = std::sync::Once::new();
19-
V8_INIT.call_once(|| {
17+
// Initialize V8 platform once and keep it alive for the lifetime of the program
18+
use std::sync::OnceLock;
19+
static V8_PLATFORM: OnceLock<v8::SharedRef<v8::Platform>> = OnceLock::new();
20+
21+
V8_PLATFORM.get_or_init(|| {
2022
let platform = v8::new_default_platform(0, false).make_shared();
21-
v8::V8::initialize_platform(platform);
23+
v8::V8::initialize_platform(platform.clone());
2224
v8::V8::initialize();
25+
platform
2326
});
2427

2528
// Compile the JavaScript to check for syntax errors
@@ -121,37 +124,48 @@ impl V8JsRuleEngine {
121124
.run(context_scope)
122125
.ok_or("Expression evaluation failed")?;
123126

124-
// Check if result is an object with 'allow' and optionally 'message'
127+
// Check if result is an object with 'allow' and/or 'deny_message'
125128
if result.is_object() {
126129
let obj = result.to_object(context_scope).unwrap();
127130

128-
// Get 'allow' property
131+
// Get 'deny_message' property if present
132+
let deny_message_key = v8::String::new(context_scope, "deny_message").unwrap();
133+
let deny_message = obj
134+
.get(context_scope, deny_message_key.into())
135+
.and_then(|v| {
136+
if v.is_undefined() || v.is_null() {
137+
None
138+
} else {
139+
v.to_string(context_scope)
140+
.map(|s| s.to_rust_string_lossy(context_scope))
141+
}
142+
});
143+
144+
// Get 'allow' property - if not present but deny_message exists, default to false
129145
let allow_key = v8::String::new(context_scope, "allow").unwrap();
130-
let allowed = obj
131-
.get(context_scope, allow_key.into())
132-
.map(|v| v.boolean_value(context_scope))
133-
.unwrap_or(false);
134-
135-
// Get 'message' property if present
136-
let message_key = v8::String::new(context_scope, "message").unwrap();
137-
let message = obj.get(context_scope, message_key.into()).and_then(|v| {
138-
if v.is_undefined() || v.is_null() {
139-
None
140-
} else {
141-
v.to_string(context_scope)
142-
.map(|s| s.to_rust_string_lossy(context_scope))
143-
}
144-
});
146+
let allow_value = obj.get(context_scope, allow_key.into());
147+
let allowed = if allow_value.is_some() && !allow_value.unwrap().is_undefined() {
148+
allow_value.unwrap().boolean_value(context_scope)
149+
} else if deny_message.is_some() {
150+
// Shorthand: if only deny_message is present, it implies allow: false
151+
false
152+
} else {
153+
// Default to false if neither is properly set
154+
false
155+
};
145156

146157
debug!(
147158
"JS rule returned object: allow={} for {} {}",
148159
allowed, request_info.method, request_info.url
149160
);
150161

151-
if let Some(ref msg) = message {
152-
debug!("Message: {}", msg);
162+
if let Some(ref msg) = deny_message {
163+
debug!("Deny message: {}", msg);
153164
}
154165

166+
// Only return the message if the request is denied
167+
let message = if !allowed { deny_message } else { None };
168+
155169
Ok((allowed, message))
156170
} else {
157171
// Result is not an object, treat as boolean
@@ -242,19 +256,18 @@ mod tests {
242256

243257
#[tokio::test]
244258
async fn test_v8_js_object_response_allow() {
245-
let engine =
246-
V8JsRuleEngine::new("({allow: true, message: 'Test allowed'})".to_string()).unwrap();
259+
let engine = V8JsRuleEngine::new("({allow: true})".to_string()).unwrap();
247260
let result = engine
248261
.evaluate(Method::GET, "https://example.com", "127.0.0.1")
249262
.await;
250263
assert!(matches!(result.action, crate::rules::Action::Allow));
251-
assert_eq!(result.context, Some("Test allowed".to_string()));
264+
assert_eq!(result.context, None); // No message when allowing
252265
}
253266

254267
#[tokio::test]
255268
async fn test_v8_js_object_response_deny() {
256269
let engine =
257-
V8JsRuleEngine::new("({allow: false, message: 'Blocked by policy'})".to_string())
270+
V8JsRuleEngine::new("({allow: false, deny_message: 'Blocked by policy'})".to_string())
258271
.unwrap();
259272
let result = engine
260273
.evaluate(Method::POST, "https://example.com", "127.0.0.1")
@@ -266,7 +279,7 @@ mod tests {
266279
#[tokio::test]
267280
async fn test_v8_js_conditional_object() {
268281
let engine = V8JsRuleEngine::new(
269-
"r.method === 'POST' ? {allow: false, message: 'POST not allowed'} : true".to_string(),
282+
"r.method === 'POST' ? {deny_message: 'POST not allowed'} : true".to_string(),
270283
)
271284
.unwrap();
272285

@@ -284,4 +297,16 @@ mod tests {
284297
assert!(matches!(result.action, crate::rules::Action::Allow));
285298
assert_eq!(result.context, None);
286299
}
300+
301+
#[tokio::test]
302+
async fn test_v8_js_shorthand_deny_message() {
303+
// Test shorthand: {deny_message: "reason"} implies allow: false
304+
let engine =
305+
V8JsRuleEngine::new("({deny_message: 'Shorthand denial'})".to_string()).unwrap();
306+
let result = engine
307+
.evaluate(Method::GET, "https://example.com", "127.0.0.1")
308+
.await;
309+
assert!(matches!(result.action, crate::rules::Action::Deny));
310+
assert_eq!(result.context, Some("Shorthand denial".to_string()));
311+
}
287312
}

0 commit comments

Comments
 (0)