Add isolated PCRE match limits as a layer of ReDoS defense #2736

brandonpayton · 2022-05-03T21:14:01Z

This PR ...

Adds support for PCRE and PCRE2 match limits as a last line of defense against ReDoS. Match limits are applied per PCRE invocation and do not change the global PCRE match limit default, which means the limits are isolated to ModSecurity and shouldn't affect other uses of PCRE in the same program.
Exposes regex errors as the variables RX_ERROR and RX_ERROR_RULE_ID so that regex engine failures can be handled by dedicated rules, rather than silently ignored. Today, if regex execution returns an error, the rx and rxGlobal operators simply fail to match. This could even be considered a security concern since such a regex error will cause a rule to be quietly bypassed.

NOTE: In the future, ModSecurity may support more proactive measures to identify vulnerable regular expressions before they can be used, but there is no harm in keeping multiple layers of defense. Doing so may even be wise, in the same way we design modern buildings to avoid fires yet still employ fire fighters for emergencies.

Background

A similar PR was rejected in the past, and if I understand correctly, it was rejected for the following reasons:

A belief that setting PCRE match limits would effect other PCRE users in the same memory space #
Users were concerned and confused about how global PCRE match limits, set for things like PHP, also affected ModSecurity #
The maintainers desire to avoid entanglement and user confusion with PCRE limits by finding other ways to avoid ReDoS #

If that is correct, I think it is worth revisiting these reasons:

Can we set match limits without affecting other PCRE users in the same memory space? Yes.
Match limits can be applied to individual PCRE pcre_exec() and PCRE2 pcre2_match() invocations without affecting other PCRE uses in the same memory space. Intuitively, if custom settings can be passed to matching functions, those settings should be limited to the function call. But we can also find evidence in the PCRE source code. In the legacy version of PCRE, the pcre_exec() function has a local match_data structure that includes the selected match limit setting. The structure is declared as part of the stack frame here. It is assigned the default match limit value here and, if specified, the custom match limit value here. Since the custom setting is maintained as part of the stack frame, it should have no global effects on other PCRE users or even other subsequent PCRE calls from ModSecurity.
Is ModSecurity affected by changes to the global PCRE default match limit? Today, the answer is yes, but there is a straightforward fix.
ModSecurity can isolate itself from PCRE's default by choosing its own default match limit (even just PCRE's normal default of 10,000,000) and always specifying that limit when invoking pcre_exec() or pcre2_match(). If ModSecurity did this for all PCRE limits instead of relying upon PCRE defaults, ModSecurity users could be isolated from the effects of modified PCRE defaults.
Whether to allow regex engine configuration is up to the ModSecurity maintainers, but some ModSecurity users want this level of configurability.
Eliminating the possibility of ReDoS prior to invoking PCRE or another regex engine is a noble goal, but such a feature does not exist in ModSecurity today. In the meantime, ModSecurity users have real ReDoS concerns that can be addressed through regular expression engine configuration, and such configuration could co-exist peacefully with more proactive anti-ReDoS measures when they are developed.

Implementation

This PR re-enables the SecPcreMatchLimit config directive, saves the match limit as a transaction property, and applies the limit in the rx and rxGlobal operators. This works with both PCRE and the experimental PCRE2 support.

This PR also adds RX_ERROR and RX_ERROR_RULE_ID variables which are set in the event of a regular expression error. RX_ERROR exposes the reason for error and can be set to either "MATCH_LIMIT" or "OTHER". RX_ERROR_RULE_ID exposes the rule ID where the regex error happened so error-handling rules can be written to respond differently based on a specific rule ID or range of rule IDs.

Here is a naive example of rules written against the RX error variables:

# Deny requests when rule IDs in the 2xxx range encounter a regex error
SecRule RX_ERROR_RULE_ID "@beginsWith 2" "id:3330,phase:2,auditlog,deny,msg:'Class 2xxx rule %{RX_ERROR_RULE_ID} encountered regex error %{RX_ERROR}',chain"
		SecRule RX_RULE_ERROR_ID "@eq 4" "t:length"

# Log when other rules encounter a regex error
SecRule RX_ERROR "@gt 0" "t:length,id:3331,phase:2,log,auditlog,allow,msg:'Rule %{RX_ERROR_RULE_ID} encountered regex error %{RX_ERROR}'"

Review

Arguably, regex engine configuration and regex error handling could be split into two PRs, but I'm starting with a single PR for the sake of discussion.

What do you think? Is this a reasonable change? Would you be willing to reconsider allowing users to set ModSecurity-specific PCRE match limits?

martinhsv · 2022-05-09T14:54:29Z

Hello @brandonpayton ,

Thanks for the contribution.

While that earlier PR predates my involvement in the project, I do know and agree that concern about limits intended for ModSecurity having effects beyond ModSecurity was indeed a reason to avoid some possible implementations. I don't know that there were any such tangible reasons associated with not considering the per-transaction overrides that you are proposing here.

I have not examined the code in this PR in detail, but I'll make a few observations to start with:

I am open to considering this; as long as the wider side-effects mentioned above can be avoided (which certainly seems to be the case, based on the documentation), this is a reasonable subject to revisit
I think I like your idea of having one or two new error variables available for this condition; without a signal like that available for additional rules to examine, we have a rather uncomfortable scenario where neither 'assume match' nor 'assume no match' can be considered a universally 'safe' option
There is, in principal, an intention to support other regex engines besides pcre/pcre2 (Hyperscan and RE2) and it's always preferable if the same options are supported in the other engines as well. However, even if that is not the case, and those per-transaction limits can only be made to work with pcre/pcre2, I'm not sure that's a good enough reason to reject it

brandonpayton · 2022-08-15T04:09:12Z

Hi @martinhsv, thank you for your feedback on this PR. I'm happy to hear you are open to changes of this nature.

There is, in principal, an intention to support other regex engines besides pcre/pcre2 (Hyperscan and RE2) and it's always preferable if the same options are supported in the other engines as well.

The regex util updates in this PR just add a basic match_limit arg to a couple of the regex utility matching methods. It sounds like that argument may need to be converted into general engine options at some point, but since the regex util is an internal implementation detail, perhaps that change can wait until it is needed.

Is there anything you'd like to see to help move this PR forward? Just a heads up: I am taking a sabbatical for the next few months and may take a while to respond, but I am still very much interested in this change.

Note: I rebased and resolved merge conflicts but haven't had a chance to test that update yet.

liudongmiao · 2022-08-18T03:57:12Z

IMO, it's better to use atomic group or possessive quantifiers for regex rules.

brandonpayton · 2022-11-18T15:42:49Z

IMO, it's better to use atomic group or possessive quantifiers for regex rules.

Hi @liudongmiao, do you mean that it would be better if rule authors wrote better, safer regular expressions? If so, I agree.

The purpose of this PR is to serve as a safety net when loading rules written by others. One example is the OWASP Core Rule Set. We have encountered issues with some of these rules in production. Of course, it would be better to make fixes to individual rules, but to avoid encountering new problems in production, it is valuable to have a safety net like these match limits.

brandonpayton · 2022-11-18T15:54:08Z

Hello @martinhsv,

Would you like me to make any changes to this PR? Is there anything I can do to help this PR or something like it to land?

My employer Automattic is interested in using this regex safety net so we can more comfortably leverage community rule sets like those from OWASP. We have seen some ReDoS issues with open source rule sets and would like to mitigate the risk.

martinhsv · 2022-11-18T22:15:31Z

Hello @brandonpayton ,

Yes, this is one of the items that I'd hoped to get back to during the coming period. I'll review this within the next few days.

martinhsv · 2022-11-29T18:52:23Z

Hi @brandonpayton ,

There are a few design assumptions here that I think might be worth considering.

The population of your two new variables is 'first wins' -- i.e. they only get set if they have not already been set.

In general ModSecurity has a 'last wins' approach. This is true for variables like MATCHED_VAR and for pcre API matches as well (a repeated group like (a.c)* with input 'abcadc' will return only 'adc' for that capture group). Using 'last wins' would also give the admin slightly more flexibility for collecting information. I.e. it's possible for the admin to either do something with the changing variable periodically, whereas with 'first wins' that additional avenue of inquiry doesn't work.

If we continue with only holding a single value, I would suggest we use 'last wins' instead. Counterarguments are welcome.

But this bleeds into some bigger questions.

I'm somewhat hesitant about the proposed new variable RX_ERROR_RULE_ID.

This is not something ModSecurity has done before. Default rule-specific processing information tends to be communicated through ModSecurity's Debug Log.

I'm not necessarily rejecting the concept of maybe providing this sort of information through a variable. After all, some installations may prefer to run without the debug log engaged, and having some basic information available in such cases may be helpful.

However, if we do wish to pursue that goal, we might wonder if we really want to restrict the admin to only knowing about 1 of possibly multiple failures. Also, we may want to think more broadly: a processing failure that is related to a specific rule may be relevant to other types of operators besides rx. Maybe we should really be considering a new variable in the form of a collection like RULE_ERROR where each key is the rule id where a failure was seen, and the value is a fixed text describing the error.

I'm not sure I've thought this through fully; and implementing an idea like that might be better done separately, as then it would have better visibility with (and potential input from) the community.

As an immediate measure to get your central goal moving forward, I'm wondering if it might be fruitful to limit the near term change to the existing model: set a single signalling variable (in ModSecurity v2, this was called MSC_PCRE_LIMITS_EXCEEDED) and leave additional information communication to the Debug Log.

Thoughts and counterarguments are welcome.

brandonpayton · 2022-12-01T16:42:17Z

Hi @martinhsv,

Thank you for your thought and guidance. What you suggested sounds reasonable and wise.

In general ModSecurity has a 'last wins' approach.

It's good to hear about "last wins" versus "first wins", and I do not believe there is any good reason for this to deviate from overall ModSecurity philosophy and practice.

I'm somewhat hesitant about the proposed new variable RX_ERROR_RULE_ID.

This is not something ModSecurity has done before. Default rule-specific processing information tends to be communicated through ModSecurity's Debug Log.

OK, it seems like we should continue favoring the Debug Log, especially for this iteration.

Maybe we should really be considering a new variable in the form of a collection like RULE_ERROR where each key is the rule id where a failure was seen, and the value is a fixed text describing the error.

At the time, I was not sure how to do something like this or whether it was possible. If the community ends up wanting something like this, it sounds good to add in a follow-up patch.

implementing an idea like that might be better done separately, as then it would have better visibility with (and potential input from) the community.

That's a good point about community input. Agreed.

As an immediate measure to get your central goal moving forward, I'm wondering if it might be fruitful to limit the near term change to the existing model: set a single signalling variable (in ModSecurity v2, this was called MSC_PCRE_LIMITS_EXCEEDED) and leave additional information communication to the Debug Log.

Do I understand correctly that we are talking about dropping the RX_ERROR and RX_ERROR_RULE_ID variables for now and adding MSC_PCRE_LIMITS_EXCEEDED as a boolean signaling variable? If so, that sounds great.

brandonpayton · 2022-12-11T00:20:53Z

Because there is also the possibility of other PCRE errors, we may need an MSC_PCRE_ERROR flag to reflect that. Otherwise, those PCRE errors will cause rules to be bypassed, and we cannot write rules to detect and handle it.

brandonpayton · 2022-12-12T03:54:55Z

I've updated this PR to use the simpler flag variables MSC_PCRE_ERRORED and MSC_PCRE_LIMITS_EXCEEDED. The first is set whenever there is any PCRE exec error, and the second is set only when hitting PCRE match limits.

I don't love using multiple flag vars but believe no PCRE exec errors should pass completely silently because it means the rule where the error occurred was effectively ignored. Perhaps, if two flag vars are undesirable, using a single MSC_PCRE_ERRORED flag along with debug logging would be sufficient for now. What do you think?

airween · 2022-12-28T21:16:05Z

I know that libmodsecurity3 has only one type of message in error log (we can call it as a "regular" message), while mod_security2 has also the "Execution error" type of messages.

I'm sure it would hard to add, but would you consider adding a similar log message (to the Nginx's error.log particularly) if the engine runs into a match limit, as like the mod_security2? (See here and here).

The line seems something like this:

...[client 1.2.3.4] ModSecurity: Rule 7fac7d451d38 [id "-"][file "/usr/share/modsecurity-crs/rules/RESPONSE-951-DATA-LEAKAGES-SQL.conf"][line "345"] - Execution error - PCRE limits exceeded (-8): (null). [hostname  "...

and appears without triggering of any rules. That would be very useful.

martinhsv

A few notes ...

Hmph. My comments seem to be gone. I'll give it another go ...

martinhsv · 2023-01-16T20:46:08Z

headers/modsecurity/rules_set_properties.h

@@ -470,6 +471,7 @@ class RulesSetProperties {
    ConfigDouble m_requestBodyLimit;
    ConfigDouble m_requestBodyNoFilesLimit;
    ConfigDouble m_responseBodyLimit;
+    ConfigInt m_pcreMatchLimit;


I'm ok with this size. 'int' should be big enough on any architecture that matters.

Note, however, the disparity between pcre1 and pcre2 (long vs. uint32_t) for this. This may another reason to avoid doing the get_default

martinhsv · 2023-01-16T20:46:12Z

headers/modsecurity/transaction.h

@@ -134,6 +134,8 @@ class TransactionAnchoredVariables {
        m_variableInboundDataError(t, "INBOUND_DATA_ERROR"),
        m_variableMatchedVar(t, "MATCHED_VAR"),
        m_variableMatchedVarName(t, "MATCHED_VAR_NAME"),
+        m_variableMscPcreErrored(t, "MSC_PCRE_ERRORED"),


Given the name of MSC_PCRE_LIMITS_EXCEEDED, I can understand the temptation to use the verb-past-tense.

But for consistency with existing variables like MULTIPART_STRICT_ERROR and INBOUND_DATA_ERROR (etc.), I would suggest dropping the 'ED' at the end.

That makes good sense. Changed the name to MSC_PCRE_ERROR.

martinhsv · 2023-01-16T20:47:05Z

src/utils/regex.cc

@@ -311,5 +378,43 @@ int Regex::search(const std::string& s) const {
 #endif
 }

+unsigned long Regex::get_default_match_limit() const {


I'm not sure I see the value in doing this.

If the ModSecurity config has not specified a limit, why do we need to do anything special here?

Why not just allow the builtin limits within PCRE to take effect?

I'm a little bit concerned especially that this code will get run for every attempt at a match -- even a relatively small number of cycles and memory jumps can add up.

Thanks for bringing this up. I agree that we can drop get_default_match_limit().

Dropped get_default_match_limit(). We are now defaulting the match_limit param to zero and only applying the match limit when it is greater than zero.

martinhsv · 2023-01-16T20:47:57Z

src/utils/regex.cc

+unsigned long Regex::get_default_match_limit() const {
+    unsigned long default_match_limit;
+#ifdef WITH_PCRE2
+    int ret = pcre2_config(PCRE2_CONFIG_MATCHLIMIT, &default_match_limit);


The doc says this should be a pointer to a uint32_t.

(Of course, this only needs to be addressed if we don't abandon this get_default functionality entirely).

martinhsv · 2023-01-16T20:49:01Z

src/utils/regex.cc

+    return default_match_limit;
+}
+
+RegexResult Regex::to_regex_result(int pcre_exec_result) const {


cppcheck may complain about this.
It doesn't do anything with an object's state (e.g. no member variables are accessed), it could be a static function.

Don't worry about this for now.

You're right. I suspect this is just the result of mental habit from working with other programming languages.

martinhsv · 2023-01-16T20:50:28Z

src/utils/regex.cc

+    Pcre2MatchContextPtr()
+        : m_match_context(pcre2_match_context_create(NULL)) {}
+
+		Pcre2MatchContextPtr(const Pcre2MatchContextPtr&) = delete;


If disallowing copy constructor, disallow copy-assignment operator too?

Thanks for pointing this out. Fixed.

martinhsv · 2023-01-16T20:58:45Z

src/operators/rx.cc

+
+    if (transaction && transaction->m_rules->m_pcreMatchLimit.m_set) {
+        unsigned long match_limit = transaction->m_rules->m_pcreMatchLimit.m_value;
+        regex_result = re->searchOneMatch(input, captures, match_limit);


If we abandon the get_default functionality, I suspect there wouldn't be a compelling reason to overload this function (or the 'Global' equivalent). A single function could either have the second argument explicitly passed something like 0 or -1 (or the second arg could be declared with a default of one of those).

I went ahead and added a default value for the match_limit param in the declarations in src/utils/regex.h and left this logic the same. My thinking is that, this way, only regex.cc needs to worry about the default value.

martinhsv

I think we're getting close. A few comments ...

brandonpayton · 2023-01-20T01:22:19Z

Thanks, @martinhsv. I just pushed some changes to address your review comments. Please let me know if you'd like something adjusted.

brandonpayton · 2023-03-08T18:45:28Z

I know that libmodsecurity3 has only one type of message in error log (we can call it as a "regular" message), while mod_security2 has also the "Execution error" type of messages.

I'm sure it would hard to add, but would you consider adding a similar log message (to the Nginx's error.log particularly) if the engine runs into a match limit, as like the mod_security2? (See here and here).

Hi @airween, thank you for mentioning this. The idea seems pretty reasonable as execution errors are generally undesired and unexpected, but I think it might be better to tackle this in a separate PR.

brandonpayton · 2023-03-08T19:32:01Z

Hi @martinhsv, is there anything left to do under this PR?

martinhsv

I've mentioned a couple of specific things in the code.

Also:

This does not implement the limit when a regex is in a place like the variable specification (e.g. 'SecRule ARGS:/^parm[0-9]/ "phase:2 ...'. I assume that's on purpose and I think that's reasonable. Regular expressions in that position are very unlikely to cause ReDoS worry.
Do you want to rebase this PR so that it's mergeable?
We should probably include an automated test for this. If you're not sure how to do this, if you have an example handy (configured limit, regex, input string), I can do so after the fact

martinhsv · 2023-04-06T16:00:33Z

src/operators/rx.cc

+
+    // FIXME: DRY regex error reporting. This logic is currently duplicated in other operators.
+    if (regex_result != Utils::RegexResult::Ok) {
+        transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset);


The error variable gets set to '1' when an error is encountered, but there is no setting to '0' otherwise -- it's just not set to anything.

ModSecurity error variables should have testable value for 'no error' and this is conventionally '0'. Often this the variable is set to either 0 or 1 upon completion of the single task per transaction (such as REQBODY_ERROR). You won't be able to do that here. The best analog is URL_ENCODED_ERROR, which is initialized to 0 in the Transaction constructors.

Same thing with m_variableMscPcreLimitsExceeded

Thanks for pointing this out. It's definitely good to explicitly set it to a sensible initial value. Done.

martinhsv · 2023-04-06T16:11:23Z

src/utils/regex.cc

 }

-bool Regex::searchGlobal(const std::string& s, std::vector<SMatchCapture>& captures) const {
+RegexResult Regex::searchGlobal(const std::string& s, std::vector<SMatchCapture>& captures, unsigned long match_limit) const {
+    const char *subject = s.c_str();


This variable is declared and initialized under both compilation configurations (WITH_PCRE2 and else), but it is only actually used in the 'else' condition.

This line should be be moved to within that 'else' (as you have done in the searchOneMatch function). cppcheck will complain otherwise.

Thanks for noticing that. Fixed.

If the rx or rxGlobal operator encounters a regex error, the RX_ERROR and RX_ERROR_RULE_ID variables are set. RX_ERROR contains a simple error code which can be either OTHER or MATCH_LIMIT. RX_ERROR_RULE_ID unsurprisingly contains the ID of the rule associated with the error. More than one rule may encounter regex errors, but only the first error is reflected in these variables.

brandonpayton · 2023-04-11T18:16:57Z

Hi @martinhsv, thank you for the review.

This does not implement the limit when a regex is in a place like the variable specification (e.g. 'SecRule ARGS:/^parm[0-9]/ "phase:2 ...'. I assume that's on purpose and I think that's reasonable. Regular expressions in that position are very unlikely to cause ReDoS worry.

Honestly, it would be nice to eventually support match limits there wherever regular expressions are used in processing transactions, but the initial focus was on the rx operators.

Do you want to rebase this PR so that it's mergeable?

Done.

We should probably include an automated test for this. If you're not sure how to do this, if you have an example handy (configured limit, regex, input string), I can do so after the fact

Initially, I did not see how to add tests, but with your encouragement, I dug a bit and found the test setup is fairly intuitive. There is now a test for the rx operator, testing the MSC_PCRE_ERROR and MSC_PCRE_LIMITS_EXCEEDED flags.

Unfortunately, I did not have any luck triggering the error condition with a test for the rxGlobal operator, at least with PCRE2, so I have not added a test there yet. It is a bit puzzling but may have something to do with the PCRE2 options used by Regex::searchGlobal().

brandonpayton · 2023-04-11T18:47:21Z

Unfortunately, I did not have any luck triggering the error condition with a test for the rxGlobal operator, at least with PCRE2, so I have not added a test there yet. It is a bit puzzling but may have something to do with the PCRE2 options used by Regex::searchGlobal().

I remember triggering such an error while testing this patch and @rxGlobal manually via ModSecurity-nginx, so I'm not sure where the disconnect is.

brandonpayton · 2023-05-06T04:00:54Z

Hi @martinhsv, regarding the QA check failures in the check-static step, I ran make check-static and found there are some warnings around an unused variables because they are only used with ms_dbg_a().

warning: src/operators/rx.cc,73,style,unreadVariable,Variable 'regex_error_str' is assigned a value that is never used.
warning: src/operators/rx.cc,75,style,unreadVariable,Variable 'regex_error_str' is assigned a value that is never used.
...
warning: src/operators/rx_global.cc,67,style,unreadVariable,Variable 'regex_error_str' is assigned a value that is never used.
warning: src/operators/rx_global.cc,69,style,unreadVariable,Variable 'regex_error_str' is assigned a value that is never used.

It looks like there is a cppcheck suppression entry for rule_with_operator.cc that appears to be for a similar reason (declaration and use with ms_dbg_a). Should I add entries for these to test/cppcheck_suppressions.txt or adjust this in some other way?

brandonpayton · 2023-05-06T04:10:58Z

Actually, I am not sure whether warnings cause the failure but haven't yet found another cause for the cppcheck failure.

martinhsv · 2023-05-11T15:16:12Z

Merged.

There were a couple of things cppcheck was flagging: a pre-existing exclusion whose line number needed adjusting and the couple of (false positive) unreadVariable alerts. I've handled those.

I've also made a small whitespace adjustment from your final revision to the PR, but I think we're all good now.

Thanks for the contribution.

Note to others that this PR incidentally advanced the bison version for generated files to 3.8.2. That had been planned for this spring in any case but was taken care of here.

brandonpayton · 2023-05-11T18:57:38Z

Great! Thanks for all your guidance and patience, @martinhsv. I've learned a lot more about ModSecurity while working on this PR.

brandonpayton force-pushed the add-regex-match-limits-and-error-reporting branch 2 times, most recently from 142bffa to a43a773 Compare August 15, 2022 03:17

brandonpayton force-pushed the add-regex-match-limits-and-error-reporting branch from 02a16fd to 5cc77e4 Compare November 18, 2022 15:26

martinhsv mentioned this pull request Nov 23, 2022

Add JIT support for PCRE2 #2791

Merged

martinhsv reviewed Jan 16, 2023

View reviewed changes

brandonpayton force-pushed the add-regex-match-limits-and-error-reporting branch from 5a5b33d to 826c3cc Compare January 20, 2023 01:13

brandonpayton requested a review from martinhsv January 20, 2023 01:22

martinhsv reviewed Apr 6, 2023

View reviewed changes

brandonpayton added 7 commits April 11, 2023 13:40

Fix some style issues in regex util header

8c4b7c1

Switch to simpler PCRE error flags

0c42ee2

Respond to code review feedback

f3d8198

Give PCRE error vars initial value

23a0e26

Move var into conditional block where used

6f1bd27

Add PCRE error tests for rx operator

d875738

brandonpayton force-pushed the add-regex-match-limits-and-error-reporting branch from 826c3cc to d875738 Compare April 11, 2023 17:47

martinhsv merged commit 09a135b into owasp-modsecurity:v3/master May 9, 2023

martinhsv mentioned this pull request May 11, 2023

How can we avoid ReDoS without trust on PCRE limits #2072

Closed

brandonpayton deleted the add-regex-match-limits-and-error-reporting branch May 11, 2023 18:57

airween mentioned this pull request May 14, 2023

Set TX:MSC_PCRE_LIMITS_EXCEEDED variable is limits exceeded #2901

Merged

azurit mentioned this pull request May 16, 2023

PCRE match limits backward incompatible behavior #2902

Closed

Add isolated PCRE match limits as a layer of ReDoS defense #2736

Add isolated PCRE match limits as a layer of ReDoS defense #2736

Conversation

brandonpayton commented May 3, 2022

Background

Implementation

Review

martinhsv commented May 9, 2022

brandonpayton commented Aug 15, 2022

liudongmiao commented Aug 18, 2022

brandonpayton commented Nov 18, 2022

brandonpayton commented Nov 18, 2022

martinhsv commented Nov 18, 2022

martinhsv commented Nov 29, 2022

brandonpayton commented Dec 1, 2022

brandonpayton commented Dec 11, 2022 • edited Loading

brandonpayton commented Dec 12, 2022

airween commented Dec 28, 2022

martinhsv left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinhsv left a comment

Choose a reason for hiding this comment

brandonpayton commented Jan 20, 2023

brandonpayton commented Mar 8, 2023

brandonpayton commented Mar 8, 2023

martinhsv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonpayton commented Apr 11, 2023 • edited Loading

brandonpayton commented Apr 11, 2023

brandonpayton commented May 6, 2023

brandonpayton commented May 6, 2023

martinhsv commented May 11, 2023

brandonpayton commented May 11, 2023

brandonpayton commented Dec 11, 2022 •

edited

Loading

martinhsv left a comment •

edited

Loading

brandonpayton commented Apr 11, 2023 •

edited

Loading