Skip to content

add ai_content_security filter#919

Merged
spacewander merged 4 commits into
mosn:mainfrom
tznbdbb:main
Sep 8, 2025
Merged

add ai_content_security filter#919
spacewander merged 4 commits into
mosn:mainfrom
tznbdbb:main

Conversation

@tznbdbb
Copy link
Copy Markdown
Contributor

@tznbdbb tznbdbb commented Aug 5, 2025

This commit introduces a new filter plugin, ai_content_security, designed to perform content moderation on both request and response bodies in HTTP streams.

Key features include:

  • Supports moderation for both streamed (text/event-stream) and non-streamed content.
  • Uses an SSE parser to handle streamed data, enabling buffering and chunking for moderation.
  • Integrates with configurable moderator and extractor components.
  • Implements buffering logic with overlap and max length control via ContentBuffer.
  • Gracefully handles blocked content with appropriate local responses (e.g., HTTP 502) or error events.

The plugin ensures that all moderated content is validated before being forwarded to the client or upstream service.

Unit and integration tests are included to ensure correctness and reliability.


Note: This is the MVP implementation.

@github-actions github-actions Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 5, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 5, 2025

Codecov Report

❌ Patch coverage is 89.56337% with 98 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.01%. Comparing base (34ad2fb) to head (1a207ed).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
plugins/plugins/aicontentsecurity/filter.go 87.86% 19 Missing and 6 partials ⚠️
plugins/plugins/aicontentsecurity/config.go 62.06% 8 Missing and 3 partials ⚠️
types/plugins/aicontentsecurity/config.go 50.00% 9 Missing and 2 partials ⚠️
.../aicontentsecurity/moderation/aliyun/moderation.go 94.54% 6 Missing and 3 partials ⚠️
.../plugins/aicontentsecurity/sseparser/sse_parser.go 95.45% 5 Missing and 4 partials ⚠️
...ns/aicontentsecurity/contentbuffer/char_counter.go 70.83% 5 Missing and 2 partials ⚠️
...tentsecurity/moderation/localservice/moderation.go 90.54% 4 Missing and 3 partials ⚠️
...ugins/plugins/aicontentsecurity/extractor/gjson.go 91.89% 4 Missing and 2 partials ⚠️
...ins/plugins/aicontentsecurity/extractor/factory.go 50.00% 3 Missing and 2 partials ⚠️
...ns/plugins/aicontentsecurity/moderation/factory.go 50.00% 3 Missing and 2 partials ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #919      +/-   ##
==========================================
+ Coverage   85.47%   86.01%   +0.54%     
==========================================
  Files         141      153      +12     
  Lines        8557     9496     +939     
==========================================
+ Hits         7314     8168     +854     
- Misses        974     1031      +57     
- Partials      269      297      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tznbdbb
Copy link
Copy Markdown
Contributor Author

tznbdbb commented Aug 5, 2025

@spacewander

@tznbdbb tznbdbb force-pushed the main branch 3 times, most recently from a77bbdd to 1339c1f Compare August 6, 2025 00:02
@spacewander
Copy link
Copy Markdown
Member

What a big PR! Please excuse me for taking a long time to review. BTW, could you make the lint pass?

@tznbdbb tznbdbb force-pushed the main branch 3 times, most recently from ebfbfeb to 0e2ffe0 Compare August 6, 2025 19:02
@tznbdbb
Copy link
Copy Markdown
Contributor Author

tznbdbb commented Aug 6, 2025

What a big PR! Please excuse me for taking a long time to review. BTW, could you make the lint pass?

Passed.
Feel free to review it whenever it's convenient for you. If you think there’s anything else I can do to help with the review, such as splitting the PR, please let me know.

if conf.ModerationTimeout > 0 {
conf.moderationTimeout = time.Duration(conf.ModerationTimeout) * time.Millisecond
} else {
conf.moderationTimeout = time.Duration(3 * int(time.Second))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
conf.moderationTimeout = time.Duration(3 * int(time.Second))
conf.moderationTimeout = 3 * time.Second

is enough. No need to add extra conversion.


func (conf *config) Init(cb api.ConfigCallbackHandler) error {
if conf.ModerationTimeout > 0 {
conf.moderationTimeout = time.Duration(conf.ModerationTimeout) * time.Millisecond
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is more go-style to use a string with unit like 30ms and parsing it with ParseDuration.

Copy link
Copy Markdown
Contributor Author

@tznbdbb tznbdbb Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified, and I plan to use regular expressions to validate the time format
string moderation_timeout = 1 [(validate.rules).string.pattern = "^\\d+(ms|s)$"];

overlapCountDelayed: true,
initialCapacity: 2049,
shrinkFactor: 2,
resizeFactor: 1.3,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why choose 2049 over 2048, and 1.3 over 2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current approach doesn’t rely on these parameters.

acc.accumulate()
acc.check(3, []string{"事件一然后是事件", "是事件二三"})
})
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Do we have a test for parsing non-utf8 chars?
  2. Could you add a test to fuzz the content buffer? Like generating random chars and checking the result many times.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

if !ok {
return false
}
return strings.HasPrefix(contentType, "text/event-stream")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"text/event-streamwhatever" could also pass this check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified

buffer := NewContentBuffer(WithMaxChars(10), WithOverlapCharNum(0))
acc := newResultAccumulator(t, buffer)

buffer.Write([]byte("事件A1"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use English as the test input so more people can understand it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified

assert.Equal(t, "line 1\nline 2", parsed.Data)
}

// TestBufferManagement_PruneAndShrink 旨在测试缓冲区的内部管理逻辑,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use English as the comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified

n = len(p.eventBoundaries)
}

if n == 0 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this branch duplicated?

Copy link
Copy Markdown
Contributor Author

@tznbdbb tznbdbb Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment thread plugins/plugins/aicontentsecurity/sseparser/sse_parser.go
remainingSize := len(c.buffer) - c.currStart
copy(c.buffer, c.buffer[c.currStart:])
c.buffer = c.buffer[:remainingSize]
c.boundaries = c.boundaries[:0]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to deal with boundaries's capacity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran some benchmarks under high load. boundaries only takes up about 1% of the memory, so I don’t think we need to worry about its capacity.

Copy link
Copy Markdown
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just comes to my mind that some providers will generate multiple contents in a response. Like reasoning_content in deepseek: https://api-docs.deepseek.com/guides/reasoning_model.

Feel free to add a note in doc and solve it in another PRs.

@tznbdbb
Copy link
Copy Markdown
Contributor Author

tznbdbb commented Aug 27, 2025

It just comes to my mind that some providers will generate multiple contents in a response. Like reasoning_content in deepseek: https://api-docs.deepseek.com/guides/reasoning_model.

Feel free to add a note in doc and solve it in another PRs.

Added in the doc

@tznbdbb tznbdbb force-pushed the main branch 6 times, most recently from acd027f to 42d0ca4 Compare August 27, 2025 20:48
@tznbdbb
Copy link
Copy Markdown
Contributor Author

tznbdbb commented Aug 27, 2025

It seems there is an issue with the e2e testing

@spacewander
Copy link
Copy Markdown
Member

suite.go:139: start waiting for deployment istio-ingressgateway in namespace istio-system, cmd: kubectl wait --timeout=5m -n istio-system deployment/istio-ingressgateway --for=condition=Available
    suite.go:143: 
        	Error Trace:	/home/runner/work/htnn/htnn/e2e/pkg/suite/suite.go:143
        	            				/home/runner/work/htnn/htnn/e2e/pkg/suite/suite.go:87
        	            				/home/runner/work/htnn/htnn/e2e/e2e_test.go:66
        	Error:      	Received unexpected error:
        	            	exit status 1
        	Test:       	TestE2E
        	Messages:   	wait for deployment istio-ingressgateway in namespace istio-system

The dataplane failed to ready. Would you check if it is caused by the change?

@tznbdbb tznbdbb force-pushed the main branch 5 times, most recently from 1307731 to 656e9dd Compare September 1, 2025 13:37
@tznbdbb
Copy link
Copy Markdown
Contributor Author

tznbdbb commented Sep 1, 2025

Ready for review.


type Extractor interface {

// SetData parse the raw data and prepare the internal state for subsequent extraction calls.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// SetData parse the raw data and prepare the internal state for subsequent extraction calls.
// SetData parses the raw data and prepares the internal state for subsequent extraction calls.

}

func (f *filter) EncodeResponse(headers api.ResponseHeaderMap, data api.BufferInstance, trailers api.ResponseTrailerMap) api.ResultAction {
return f.streamDataHandler(data, true)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename streamDataHandler to sseDataHandler, since it will be used to parse buffered SSE messages.

)

func TestRequestModeration_live(t *testing.T) {
t.Skip("Skipping live test to avoid dependency on external services")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the live test here? We already have the integration test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can help users debug the locally built reasoning API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hasn't the integration test already done the same thing? And the live test is not run in the CI at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, i will remove it.

- AUDIT_PORT=10902
restart: unless-stopped
volumes:
- ./ai_content_security/app.py:/app/app.py # 挂载本地文件到容器
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- ./ai_content_security/app.py:/app/app.py # 挂载本地文件到容器
- ./ai_content_security/app.py:/app/app.py # Mount local files into the container

Comment thread plugins/plugins/aicontentsecurity/sseparser/sse_parser.go
Comment thread plugins/plugins/aicontentsecurity/sseparser/sse_parser.go
Comment thread plugins/plugins/aicontentsecurity/sseparser/sse_parser.go
@tznbdbb tznbdbb force-pushed the main branch 2 times, most recently from f9b1a81 to 8fcdd15 Compare September 5, 2025 12:26
@spacewander spacewander merged commit a3145f5 into mosn:main Sep 8, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants