Skip to content

Commit

Permalink
filtering: imp code, tests
Browse files Browse the repository at this point in the history
  • Loading branch information
EugeneOne1 committed Oct 19, 2022
1 parent be56df7 commit 2e57624
Show file tree
Hide file tree
Showing 5 changed files with 197 additions and 23 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ and this project adheres to

### Fixed

- Editing an enabled rule list's URL now also includes validation of the filter
contents preventing from saving a bad one ([#4916]).
- The default value of `dns.cache_size` accidentally set to 0 has now been
reverted to 4 MiB ([#5010]).
- Responses for which the DNSSEC validation had explicitly been omitted aren't
Expand All @@ -38,6 +40,7 @@ and this project adheres to

[#2926]: https://github.com/AdguardTeam/AdGuardHome/issues/2926
[#3418]: https://github.com/AdguardTeam/AdGuardHome/issues/3418
[#4916]: https://github.com/AdguardTeam/AdGuardHome/issues/4916
[#4925]: https://github.com/AdguardTeam/AdGuardHome/issues/4925
[#4942]: https://github.com/AdguardTeam/AdGuardHome/issues/4942
[#4986]: https://github.com/AdguardTeam/AdGuardHome/issues/4986
Expand Down
45 changes: 29 additions & 16 deletions internal/filtering/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,62 +56,72 @@ func (filter *FilterYAML) Path(dataDir string) string {
const (
// errFilterNotExist is returned from [filterSetProperties] when there are
// no lists with the desired URL to update.
//
// TODO(e.burkov): Use wherever the same error is needed.
errFilterNotExist errors.Error = "url doesn't exist"

// errFilterExists is returned from [filterSetProperties] when there is
// another filter having the same URL as the one updated.
//
// TODO(e.burkov): Use wherever the same error is needed.
errFilterExists errors.Error = "url already exists"
)

// filterSetProperties searches for the particular filter list by url and sets
// the values of newf to it, updating afterwards if needed. It returns true if
// the update was performed and the filtering engine is needed to be restarted.
func (d *DNSFilter) filterSetProperties(
url string,
newf FilterYAML,
white bool,
listURL string,
newList FilterYAML,
isAllowlist bool,
) (restart bool, err error) {
d.filtersMu.Lock()
defer d.filtersMu.Unlock()

filters := d.Filters
if white {
if isAllowlist {
filters = d.WhitelistFilters
}

i := slices.IndexFunc(filters, func(filt FilterYAML) bool { return filt.URL == url })
i := slices.IndexFunc(filters, func(filt FilterYAML) bool { return filt.URL == listURL })
if i == -1 {
return false, errFilterNotExist
}

filt := &filters[i]

log.Debug("filter: set properties: %s: {%s %s %v}", filt.URL, newf.Name, newf.URL, newf.Enabled)

defer func(oldURL, oldName string, oldEnabled bool) {
log.Debug(
"filter: set properties: %s: {%s %s %v}",
filt.URL,
newList.Name,
newList.URL,
newList.Enabled,
)

defer func(oldURL, oldName string, oldEnabled bool, oldUpdated time.Time) {
if err != nil {
filt.URL = oldURL
filt.Name = oldName
filt.Enabled = oldEnabled
filt.LastUpdated = oldUpdated
}
}(filt.URL, filt.Name, filt.Enabled)
}(filt.URL, filt.Name, filt.Enabled, filt.LastUpdated)

filt.Name = newf.Name
filt.Name = newList.Name

if filt.URL != newf.URL {
if d.filterExistsNoLock(newf.URL) {
if filt.URL != newList.URL {
if d.filterExistsNoLock(newList.URL) {
return false, errFilterExists
}

restart = true

filt.URL = newf.URL
filt.URL = newList.URL
filt.LastUpdated = time.Time{}
filt.unload()
}

if filt.Enabled != newf.Enabled {
filt.Enabled = newf.Enabled
if filt.Enabled != newList.Enabled {
filt.Enabled = newList.Enabled
restart = true
}

Expand All @@ -121,6 +131,9 @@ func (d *DNSFilter) filterSetProperties(
restart, err = d.update(filt)
}
} else {
// TODO(e.burkov): Validating of the new URL's content is skipped if
// the filter is turned off. It makes possible to set the malformed
// rules source, but will disallow to enable the filter afterwards.
filt.unload()
}

Expand Down
14 changes: 9 additions & 5 deletions internal/filtering/filter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,26 @@ import (

const testFltsFileName = "1.txt"

// testStartFilterListener is a helper that concurrently listens on a free port
// to respond with fltContent.
//
// TODO(e.burkov): Remove pointer to the slice of bytes.
func testStartFilterListener(t *testing.T, fltContent *[]byte) (l net.Listener) {
t.Helper()

h := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
pt := testutil.PanicT{}

n, werr := w.Write(*fltContent)
require.NoError(t, werr)
require.Equal(t, len(*fltContent), n)
require.NoError(pt, werr)
require.Equal(pt, len(*fltContent), n)
})

var err error
l, err = net.Listen("tcp", ":0")
require.NoError(t, err)

go func() {
_ = http.Serve(l, h)
}()
go func() { _ = http.Serve(l, h) }()
testutil.CleanupAndRequireSuccess(t, l.Close)

return l
Expand Down
3 changes: 1 addition & 2 deletions internal/filtering/filtering.go
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,7 @@ func (d *DNSFilter) SetFilters(blockFilters, allowFilters []Filter, async bool)
defer d.filtersInitializerLock.Unlock()

// remove all pending tasks
stop := false
for !stop {
for stop := false; !stop; {
select {
case <-d.filtersInitializerChan:
//
Expand Down
155 changes: 155 additions & 0 deletions internal/filtering/http_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
package filtering

import (
"bytes"
"encoding/json"
"net"
"net/http"
"net/http/httptest"
"net/netip"
"net/url"
"testing"
"time"

"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestDNSFilter_handleFilteringSetURL(t *testing.T) {
filtersDir := t.TempDir()

var goodRulesEndpoint, anotherGoodRulesEndpoint, badRulesEndpoint string
for _, rulesSource := range []struct {
endpoint *string
content []byte
}{{
endpoint: &goodRulesEndpoint,
content: []byte(`||example.org^`),
}, {
endpoint: &anotherGoodRulesEndpoint,
content: []byte(`||example.com^`),
}, {
endpoint: &badRulesEndpoint,
content: []byte(`<html></html>`),
}} {
rulesSource := rulesSource
listener := testStartFilterListener(t, &rulesSource.content)

addr := listener.Addr()
require.IsType(t, new(net.TCPAddr), addr)
port := addr.(*net.TCPAddr).AddrPort().Port()

*rulesSource.endpoint = (&url.URL{
Scheme: "http",
Host: netip.AddrPortFrom(aghnet.IPv4Localhost(), port).String(),
}).String()
}

testCases := []struct {
name string
wantBody string
oldURL string
newName string
newURL string
initial []FilterYAML
}{{
name: "success",
wantBody: "",
oldURL: goodRulesEndpoint,
newName: "default_one",
newURL: anotherGoodRulesEndpoint,
initial: []FilterYAML{{
Enabled: true,
URL: goodRulesEndpoint,
Name: "default_one",
white: false,
}},
}, {
name: "non-existing",
wantBody: "url doesn't exist\n",
oldURL: anotherGoodRulesEndpoint,
newName: "default_one",
newURL: goodRulesEndpoint,
initial: []FilterYAML{{
Enabled: true,
URL: goodRulesEndpoint,
Name: "default_one",
white: false,
}},
}, {
name: "existing",
wantBody: "url already exists\n",
oldURL: goodRulesEndpoint,
newName: "default_one",
newURL: anotherGoodRulesEndpoint,
initial: []FilterYAML{{
Enabled: true,
URL: goodRulesEndpoint,
Name: "default_one",
white: false,
}, {
Enabled: true,
URL: anotherGoodRulesEndpoint,
Name: "another_default_one",
white: false,
}},
}, {
name: "bad_rules",
wantBody: "data is HTML, not plain text\n",
oldURL: goodRulesEndpoint,
newName: "default_one",
newURL: badRulesEndpoint,
initial: []FilterYAML{{
Enabled: true,
URL: goodRulesEndpoint,
Name: "default_one",
white: false,
}},
}}

for _, tc := range testCases {
tc := tc

t.Run(tc.name, func(t *testing.T) {
configModified := false
d, err := New(&Config{
FilteringEnabled: true,
Filters: tc.initial,
HTTPClient: &http.Client{
Timeout: 5 * time.Second,
},
ConfigModified: func() { configModified = true },
DataDir: filtersDir,
}, nil)
require.NoError(t, err)
t.Cleanup(d.Close)

d.Start()

reqData := &filterURLReq{
Data: &filterURLReqData{
// Leave the name of an existing list.
Name: tc.newName,
URL: tc.newURL,
Enabled: true,
},
URL: tc.oldURL,
Whitelist: false,
}
data, err := json.Marshal(reqData)
require.NoError(t, err)

r := httptest.NewRequest(http.MethodPost, "http://example.org", bytes.NewReader(data))
w := httptest.NewRecorder()

d.handleFilteringSetURL(w, r)
assert.Equal(t, tc.wantBody, w.Body.String())
if tc.wantBody != "" {
assert.False(t, configModified)
} else {
assert.True(t, configModified)
}
})
}
}

0 comments on commit 2e57624

Please sign in to comment.